Hi
To make sure headers don't create conflicts, guards are added to all of them. But the guard needs to be correct: e.g. https://review.coreboot.org/c/coreboot/+/64360/2 Most compilers implement '#pragma once ' as an alternative. Should we use this instead across the tree, as it is less error prone and less code?
Sidenote: clang warns about wrong header guards. https://review.coreboot.org/c/coreboot/+/62173/23 hooks up clang to our CI for some platforms ;-).
Kind regards Arthur
On Sun, May 15, 2022 at 11:56 AM Arthur Heymans arthur@aheymans.xyz wrote:
Hi
To make sure headers don't create conflicts, guards are added to all of them. But the guard needs to be correct: e.g. https://review.coreboot.org/c/coreboot/+/64360/2
A few more that I noticed by grepping around: https://review.coreboot.org/c/coreboot/+/64364
Most compilers implement '#pragma once ' as an alternative. Should we use this instead across the tree, as it is less error prone and less code?
Sounds good to me. It will also mitigate awkward guards with paths in them, for example in src/mainboard/google/smaug/pmic.h which uses `#ifndef __MAINBOARD_GOOGLE_FOSTER_PMIC_H__`. (This should probably be factored out into a common header that smaug and foster include, but that's another matter)
On Sun, 15 May 2022 at 19:56, Arthur Heymans arthur@aheymans.xyz wrote:
Most compilers implement '#pragma once ' as an alternative.
I've been using #pragma once since 2019 in fwupd and I've never once had a problem with it -- and we compile with a lot of weird compilers and for a lot of strange targets. It reduced the amount of boilerplate by a huge amount, and removed one new-contributor "gotcha" that was lurking for every person that added a new header.
Richard.
Hi Arthur, list,
On Sun, May 15, 2022 at 6:56 PM Arthur Heymans arthur@aheymans.xyz wrote:
Hi
To make sure headers don't create conflicts, guards are added to all of them. But the guard needs to be correct: e.g. https://review.coreboot.org/c/coreboot/+/64360/2 Most compilers implement '#pragma once ' as an alternative. Should we use this instead across the tree, as it is less error prone and less code?
Given that coreboot is built with a very specific toolchain, it seems very reasonable. The only thing that worries me are headers used to build stuff with the system toolchain, e.g. util/ and src/commonlib/ headers. Still, it's highly unlikely that the system toolchain doesn't know about #pragma once provided that it is able to build crossgcc.
Sidenote: clang warns about wrong header guards. https://review.coreboot.org/c/coreboot/+/62173/23 hooks up clang to our CI for some platforms ;-).
And mismatched names in #ifndef and #define is not the only problem. I recently pondered about the scenario in which a compilation unit includes two different header files that use the same name in their guard. Using #pragma once would fundamentally eliminate both problems.
Kind regards Arthur
Best regards, Angel
btw, sometimes this has gone the other direction .. https://github.com/lowRISC/opentitan/pull/5693
On Mon, May 16, 2022 at 3:04 AM Angel Pons th3fanbus@gmail.com wrote:
Hi Arthur, list,
On Sun, May 15, 2022 at 6:56 PM Arthur Heymans arthur@aheymans.xyz wrote:
Hi
To make sure headers don't create conflicts, guards are added to all of them. But the guard needs to be correct: e.g. https://review.coreboot.org/c/coreboot/+/64360/2 Most compilers implement '#pragma once ' as an alternative. Should we use this instead across the tree, as it is less error prone and less code?
Given that coreboot is built with a very specific toolchain, it seems very reasonable. The only thing that worries me are headers used to build stuff with the system toolchain, e.g. util/ and src/commonlib/ headers. Still, it's highly unlikely that the system toolchain doesn't know about #pragma once provided that it is able to build crossgcc.
Sidenote: clang warns about wrong header guards. https://review.coreboot.org/c/coreboot/+/62173/23 hooks up clang to our CI for some platforms ;-).
And mismatched names in #ifndef and #define is not the only problem. I recently pondered about the scenario in which a compilation unit includes two different header files that use the same name in their guard. Using #pragma once would fundamentally eliminate both problems.
Kind regards Arthur
Best regards, Angel _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
On Mon, May 16, 2022 at 8:59 AM ron minnich rminnich@gmail.com wrote:
btw, sometimes this has gone the other direction .. https://github.com/lowRISC/opentitan/pull/5693
It looks like they did that solely to conform to Google's style guide which, dogmatic as it may appear, makes sense since OpenTitan is a Google-lead project.
I was sure I'd looked at this at one point and found this from years ago ...
"The discussion evolved to a related question, around #pragma once. A few years back, on the Akaros project (kernel written in C, FWIW), a Linux kernel luminary convinced us to get rid of file guards and go to #pragma once. I am not sure it was worth the trouble but we did it. It *can* speed up compile time; cpp doesn't need to process a whole file and then conclude it did not have to process it; it can realize it can skip the open. A significant downside is that it's not in any standard -- just all the compilers out there, it seems, save romcc.
I did a simple test: apply #pragma once to coreboot. A coreboot build for watson opens 80K .h files today. #pragma once makes barely any difference; this says we are doing a good job in how we use our .h files."
Anyway, all this said, #pragma once seems a good idea.
On Mon, May 16, 2022 at 9:59 AM David Hendricks david.hendricks@gmail.com wrote:
On Mon, May 16, 2022 at 8:59 AM ron minnich rminnich@gmail.com wrote:
btw, sometimes this has gone the other direction .. https://github.com/lowRISC/opentitan/pull/5693
It looks like they did that solely to conform to Google's style guide which, dogmatic as it may appear, makes sense since OpenTitan is a Google-lead project.
After reading what I've included below, I'm going to have to vote to keep the guards. Martin
May 16, 2022, 10:59 by david.hendricks@gmail.com:
On Mon, May 16, 2022 at 8:59 AM ron minnich rminnich@gmail.com wrote:
btw, sometimes this has gone the other direction .. https://github.com/lowRISC/opentitan/pull/5693
It looks like they did that solely to conform to Google's style guide which, dogmatic as it may appear, makes sense since OpenTitan is a Google-lead project.
The question then is 'why does Google require the use of guards?'. Whatever you think of google, they're not going to mandate something like this without a good reason.
I went searching for where this rule came from, and found this:
``` If you trust our in-house C++ compiler gurus, here's the most salient part of the whole thread linked above.Matt Austern (4/11/2013): If you talk to the authors of [most C++] compilers, I think you'll find that most of them consider "#pragma once" (or the equivalent #import) to be a flaky feature -- something that works almost all of the time and that can cause seriously annoying bugs when it doesn't work.
Chandler Carruth (4/12/2013): As one of the authors of one of those compilers, I couldn't agree more. ``` any interested Googlers can find this here: https://yaqs.corp.google.com/eng/q/5768278562045952
Further digging: ``` To support #pragma once, the compiler tries to identify duplicate encounters with the same file, but the check gcc actually performs to establish the identity of the file is weak. Here's someone who made two copies of the same header with different names, each with a #pragma once, and it screwed up his build.
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52566
The two headers had the same size, same content, and same timestamps because he had run "touch *" on them, but they were intended to both be included. Only one was included, and the other was falsely identified as a re-inclusion and ignored.One might say he was asking for trouble by running "touch *", but timestamp collisions are EASY to come by. First of all, they're only 1sec resolution. You might patch all the relevant files and they'd have matching timestamps. You might be using a network filesystem that just doesn't bother with timestamps (common). ```
Now both of these are almost a decade old, so things might have changed quite a bit since then.
Linux kernel threads: https://lkml.iu.edu/hypermail/linux/kernel/1401.0/02048.html https://lore.kernel.org/lkml/CAHk-=wi54DEScexxpMrO+Q2Nag_Tup+Y5YBHc_9_xGLeRf...
``` On Sun, Feb 28, 2021 at 11:34 AM Alexey Dobriyan adobriyan@gmail.com wrote:>> >> > End result: #pragma is fundamentally less reliable than the> > traditional #ifdef guard. The #ifdef guard works fine even if you> > re-read the file for whatever reason, while #pragma relies on some> > kind of magical behavior.You continue to not even answer this very fundamental question."#pragma once" doesn't seem to have a _single_ actual real advantage.Everybody already does the optimization of not even opening - muchless reading and re-parsing - headers that have the traditional #ifdefguard.And even if you _don't_ do that optimization, the #ifdef guardfundmentally semantically guarantyees the right behavior.So the #ifdef guard is (a) standard (b) simple (c) reliable (d) traditionaland you have yet to explain a _single_ advantage of "#pragma once".Why add this incredible churn that has no upside?So no. We're not using #pragma once unless y9ou can come up with somevery strong argument for itAnd no, having to come up with a name for the #ifdef guard is not astrong argument. It's simply not that complicated. Linus ```
we have, in the past, used Linux kernel style as our guideline on coreboot style.
I'd say, based on Martin's note, that #pragma once is not such a good idea after all. If we decide NOT to use it, however, let's put a note about it in our style guide?
This is not the first time this question has come up.
On Mon, May 16, 2022 at 12:34 PM Martin Roth gaumless@tutanota.com wrote:
After reading what I've included below, I'm going to have to vote to keep the guards. Martin
May 16, 2022, 10:59 by david.hendricks@gmail.com:
On Mon, May 16, 2022 at 8:59 AM ron minnich rminnich@gmail.com wrote:
btw, sometimes this has gone the other direction .. https://github.com/lowRISC/opentitan/pull/5693
It looks like they did that solely to conform to Google's style guide which, dogmatic as it may appear, makes sense since OpenTitan is a Google-lead project.
The question then is 'why does Google require the use of guards?'. Whatever you think of google, they're not going to mandate something like this without a good reason.
I went searching for where this rule came from, and found this:
If you trust our in-house C++ compiler gurus, here's the most salient part of the whole thread linked above.Matt Austern (4/11/2013): If you talk to the authors of [most C++] compilers, I think you'll find that most of them consider "#pragma once" (or the equivalent #import) to be a flaky feature -- something that works almost all of the time and that can cause seriously annoying bugs when it doesn't work. Chandler Carruth (4/12/2013): As one of the authors of one of those compilers, I couldn't agree more.any interested Googlers can find this here: https://yaqs.corp.google.com/eng/q/5768278562045952
Further digging:
To support #pragma once, the compiler tries to identify duplicate encounters with the same file, but the check gcc actually performs to establish the identity of the file is weak. Here's someone who made two copies of the same header with different names, each with a #pragma once, and it screwed up his build. http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52566 The two headers had the same size, same content, and same timestamps because he had run "touch *" on them, but they were intended to both be included. Only one was included, and the other was falsely identified as a re-inclusion and ignored.One might say he was asking for trouble by running "touch *", but timestamp collisions are EASY to come by. First of all, they're only 1sec resolution. You might patch all the relevant files and they'd have matching timestamps. You might be using a network filesystem that just doesn't bother with timestamps (common).Now both of these are almost a decade old, so things might have changed quite a bit since then.
Linux kernel threads: https://lkml.iu.edu/hypermail/linux/kernel/1401.0/02048.html https://lore.kernel.org/lkml/CAHk-=wi54DEScexxpMrO+Q2Nag_Tup+Y5YBHc_9_xGLeRf...
On Sun, Feb 28, 2021 at 11:34 AM Alexey Dobriyan <adobriyan@gmail.com> wrote:>> >> > End result: #pragma is fundamentally less reliable than the> > traditional #ifdef guard. The #ifdef guard works fine even if you> > re-read the file for whatever reason, while #pragma relies on some> > kind of magical behavior.You continue to not even answer this very fundamental question."#pragma once" doesn't seem to have a _single_ actual real advantage.Everybody already does the optimization of not even opening - muchless reading and re-parsing - headers that have the traditional #ifdefguard.And even if you _don't_ do that optimization, the #ifdef guardfundmentally semantically guarantyees the right behavior.So the #ifdef guard is (a) standard (b) simple (c) reliable (d) traditionaland you have yet to explain a _single_ advantage of "#pragma once".Why add this incredible churn that has no upside?So no. We're not using #pragma once unless y9ou can come up with somevery strong argument for itAnd no, having to come up with a name for the #ifdef guard is not astrong argument. It's simply not that complicated. Linus
Thanks for digging up all that useful information, Martin! So `#pragma once` is not the clear winner after all. Too bad since we could eliminate some boilerplate code with that approach.
Is there another way to solve the problem Arthur raised in this thread? The LMKL thread had a python script and people were also talking about adding something to checkpatch.pl. Maybe these days it's sufficient to assume clang will catch any real problems (CB:62173).
On Mon, May 16, 2022 at 1:03 PM ron minnich rminnich@gmail.com wrote:
we have, in the past, used Linux kernel style as our guideline on coreboot style.
I'd say, based on Martin's note, that #pragma once is not such a good idea after all. If we decide NOT to use it, however, let's put a note about it in our style guide?
This is not the first time this question has come up.
On Mon, May 16, 2022 at 12:34 PM Martin Roth gaumless@tutanota.com wrote:
After reading what I've included below, I'm going to have to vote to
keep the guards.
Martin
May 16, 2022, 10:59 by david.hendricks@gmail.com:
On Mon, May 16, 2022 at 8:59 AM ron minnich rminnich@gmail.com
wrote:
btw, sometimes this has gone the other direction .. https://github.com/lowRISC/opentitan/pull/5693
It looks like they did that solely to conform to Google's style guide which, dogmatic as it may appear, makes sense since OpenTitan is a Google-lead project.
The question then is 'why does Google require the use of guards?'.
Whatever you think of google, they're not going to mandate something like this without a good reason.
I went searching for where this rule came from, and found this:
If you trust our in-house C++ compiler gurus, here's the most salientpart of the whole thread linked above.Matt Austern (4/11/2013): If you talk to the authors of [most C++] compilers, I think you'll find that most of them consider "#pragma once" (or the equivalent #import) to be a flaky feature -- something that works almost all of the time and that can cause seriously annoying bugs when it doesn't work.
Chandler Carruth (4/12/2013): As one of the authors of one of those
compilers, I couldn't agree more.
any interested Googlers can find this here: https://yaqs.corp.google.com/eng/q/5768278562045952 Further digging:To support #pragma once, the compiler tries to identify duplicate
encounters with the same file, but the check gcc actually performs to establish the identity of the file is weak. Here's someone who made two copies of the same header with different names, each with a #pragma once, and it screwed up his build.
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52566
The two headers had the same size, same content, and same timestamps
because he had run "touch *" on them, but they were intended to both be included. Only one was included, and the other was falsely identified as a re-inclusion and ignored.One might say he was asking for trouble by running "touch *", but timestamp collisions are EASY to come by. First of all, they're only 1sec resolution. You might patch all the relevant files and they'd have matching timestamps. You might be using a network filesystem that just doesn't bother with timestamps (common).
Now both of these are almost a decade old, so things might have changedquite a bit since then.
Linux kernel threads: https://lkml.iu.edu/hypermail/linux/kernel/1401.0/02048.html
https://lore.kernel.org/lkml/CAHk-=wi54DEScexxpMrO+Q2Nag_Tup+Y5YBHc_9_xGLeRf...
On Sun, Feb 28, 2021 at 11:34 AM Alexey Dobriyan <adobriyan@gmail.com>wrote:>> >> > End result: #pragma is fundamentally less reliable than the>
traditional #ifdef guard. The #ifdef guard works fine even if you> >
re-read the file for whatever reason, while #pragma relies on some> > kind of magical behavior.You continue to not even answer this very fundamental question."#pragma once" doesn't seem to have a _single_ actual real advantage.Everybody already does the optimization of not even opening - muchless reading and re-parsing - headers that have the traditional #ifdefguard.And even if you _don't_ do that optimization, the #ifdef guardfundmentally semantically guarantyees the right behavior.So the #ifdef guard is (a) standard (b) simple (c) reliable (d) traditionaland you have yet to explain a _single_ advantage of "#pragma once".Why add this incredible churn that has no upside?So no. We're not using #pragma once unless y9ou can come up with somevery strong argument for itAnd no, having to come up with a name for the #ifdef guard is not astrong argument. It's simply not that complicated. Linus
David, apropos your comment that we can assume clang will catch any real problems; your comment reminds me of a lesson we learned on Harvey-OS.
What we learned on Harvey-OS, when we were compiling a working kernel across 3 versions of gcc, and 3 versions of clang, was that at one time or another, one of these 6 compilers gave us a warning on some critical code issue in Harvey-0S, that the other 5 did not. Older compilers warned of something a newer one did not; gcc and clang each had warnings the other missed.
This was surprising and a useful lesson to us. It's also why I'll never be able to accept the idea that coreboot should be written in a gcc-specific dialect. But that's another argument for another day.
On Mon, May 16, 2022 at 9:53 PM David Hendricks david.hendricks@gmail.com wrote:
Thanks for digging up all that useful information, Martin! So `#pragma once` is not the clear winner after all. Too bad since we could eliminate some boilerplate code with that approach.
Is there another way to solve the problem Arthur raised in this thread? The LMKL thread had a python script and people were also talking about adding something to checkpatch.pl. Maybe these days it's sufficient to assume clang will catch any real problems (CB:62173).
On Mon, May 16, 2022 at 1:03 PM ron minnich rminnich@gmail.com wrote:
we have, in the past, used Linux kernel style as our guideline on coreboot style.
I'd say, based on Martin's note, that #pragma once is not such a good idea after all. If we decide NOT to use it, however, let's put a note about it in our style guide?
This is not the first time this question has come up.
On Mon, May 16, 2022 at 12:34 PM Martin Roth gaumless@tutanota.com wrote:
After reading what I've included below, I'm going to have to vote to keep the guards. Martin
May 16, 2022, 10:59 by david.hendricks@gmail.com:
On Mon, May 16, 2022 at 8:59 AM ron minnich rminnich@gmail.com wrote:
btw, sometimes this has gone the other direction .. https://github.com/lowRISC/opentitan/pull/5693
It looks like they did that solely to conform to Google's style guide which, dogmatic as it may appear, makes sense since OpenTitan is a Google-lead project.
The question then is 'why does Google require the use of guards?'. Whatever you think of google, they're not going to mandate something like this without a good reason.
I went searching for where this rule came from, and found this:
If you trust our in-house C++ compiler gurus, here's the most salient part of the whole thread linked above.Matt Austern (4/11/2013): If you talk to the authors of [most C++] compilers, I think you'll find that most of them consider "#pragma once" (or the equivalent #import) to be a flaky feature -- something that works almost all of the time and that can cause seriously annoying bugs when it doesn't work. Chandler Carruth (4/12/2013): As one of the authors of one of those compilers, I couldn't agree more.any interested Googlers can find this here: https://yaqs.corp.google.com/eng/q/5768278562045952
Further digging:
To support #pragma once, the compiler tries to identify duplicate encounters with the same file, but the check gcc actually performs to establish the identity of the file is weak. Here's someone who made two copies of the same header with different names, each with a #pragma once, and it screwed up his build. http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52566 The two headers had the same size, same content, and same timestamps because he had run "touch *" on them, but they were intended to both be included. Only one was included, and the other was falsely identified as a re-inclusion and ignored.One might say he was asking for trouble by running "touch *", but timestamp collisions are EASY to come by. First of all, they're only 1sec resolution. You might patch all the relevant files and they'd have matching timestamps. You might be using a network filesystem that just doesn't bother with timestamps (common).Now both of these are almost a decade old, so things might have changed quite a bit since then.
Linux kernel threads: https://lkml.iu.edu/hypermail/linux/kernel/1401.0/02048.html https://lore.kernel.org/lkml/CAHk-=wi54DEScexxpMrO+Q2Nag_Tup+Y5YBHc_9_xGLeRf...
On Sun, Feb 28, 2021 at 11:34 AM Alexey Dobriyan <adobriyan@gmail.com> wrote:>> >> > End result: #pragma is fundamentally less reliable than the> > traditional #ifdef guard. The #ifdef guard works fine even if you> > re-read the file for whatever reason, while #pragma relies on some> > kind of magical behavior.You continue to not even answer this very fundamental question."#pragma once" doesn't seem to have a _single_ actual real advantage.Everybody already does the optimization of not even opening - muchless reading and re-parsing - headers that have the traditional #ifdefguard.And even if you _don't_ do that optimization, the #ifdef guardfundmentally semantically guarantyees the right behavior.So the #ifdef guard is (a) standard (b) simple (c) reliable (d) traditionaland you have yet to explain a _single_ advantage of "#pragma once".Why add this incredible churn that has no upside?So no. We're not using #pragma once unless y9ou can come up with somevery strong argument for itAnd no, having to come up with a name for the #ifdef guard is not astrong argument. It's simply not that complicated. Linus
Hi Martin,
To support #pragma once, the compiler tries to identify duplicate encounters with the same file, but the check gcc actually performs to establish the identity of the file is weak. Here's someone who made two copies of the same header with different names, each with a #pragma once, and it screwed up his build.
Ouch, that isn't what I expected here; especially since multiple files with the same timestamp are expected when doing a fresh repo checkout. With this info I agree that we should keep the include guard; definitely learned something new today. It would be helpful to have this documented and possibly have some check to make sure that the include guards aren't broken in some files.
Regards, Felix
Hi
Those arguments not to use #pragma once make a lot of sense. Thanks Martin!
I've made some good progress on getting boards to build with clang (each x86 board now builds). Clang at least warns about #ifndef and #define lines not being equal so we'd have that check covered.
Kind regards Arthur
On Tue, 17 May 2022, 15:28 Felix Held, felix-coreboot@felixheld.de wrote:
Hi Martin,
To support #pragma once, the compiler tries to identify duplicate
encounters with the same file, but the check gcc actually performs to establish the identity of the file is weak. Here's someone who made two copies of the same header with different names, each with a #pragma once, and it screwed up his build.
Ouch, that isn't what I expected here; especially since multiple files with the same timestamp are expected when doing a fresh repo checkout. With this info I agree that we should keep the include guard; definitely learned something new today. It would be helpful to have this documented and possibly have some check to make sure that the include guards aren't broken in some files.
Regards, Felix _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
Hi Arthur,
I'm very much in favor of using #pragma once instead of the include guards in the coreboot tree, since that removes the possibility of some sorts of bugs and also removes 2 lines of boilerplate per header file. Not sure if romcc supported #pragma once and if that was one of the reasons to use the include guards instead, but since romcc was dropped from the main branch a long time ago, that's not a problem.
Regards, Felix
-
Angel Pons -
Arthur Heymans -
David Hendricks -
Felix Held -
Martin Roth -
Richard Hughes -
ron minnich