On 2/8/20 7:10 PM, Qian Cai wrote:
On Feb 8, 2020, at 8:44 PM, John Hubbard <jhubbard@xxxxxxxxxx> wrote:
So it looks like we're probably stuck with having to annotate the code. Given
that, there is a balance between how many macros, and how much commenting. For
example, if there is a single macro (data_race, for example), then we'll need to
add comments for the various cases, explaining which data_race situation is
happening.
On the other hand, it is perfect fine of not commenting on each data_race() that most of times, people could run git blame to learn more details. Actually, no maintainers from various of subsystems asked for commenting so far.
Well, maybe I'm looking at this wrong. I was thinking that one should attempt to
understand the code on the screen, and that's generally best--but here, maybe
"data_race" is just something that means "tool cruft", really. So mentally we
would move toward visually filtering out the data_race "key word".
I really don't like it but at least there is a significant benefit from the tool
that probably makes it worth the visual noise.
Blue sky thoughts for The Far Future: It would be nice if the tools got a lot
better--maybe in the direction of C language extensions, even if only used in
this project at first.
thanks,
--
John Hubbard
NVIDIA
That's still true, but to a lesser extent if more macros are added. In this case,
I suspect that READ_BITS() makes the commenting easier and shorter. So I'd tentatively
lead towards adding it, but what do others on the list think?
Even read bits could be dangerous from data races and confusing at best, so I am not really sure what the value of introducing this new macro. People who like to understand it correctly still need to read the commit logs.
This flags->zonenum is such a special case that I don’t really see it regularly for the last few weeks digging KCSAN reports, so even if it is worth adding READ_BITS(), there are more equally important macros need to be added together to be useful initially. For example, HARMLESS_COUNTERS(), READ_SINGLE_BIT(), READ_IMMUTATABLE_BITS() etc which Linus said exactly wanted to avoid.
