| Message ID | gerrit.1574340892000.Id4b8ee19176a9e4624b533087ba870c418f27e60@gnutoolchain-gerrit.osci.io |
|---|---|
| State | New |
| Headers | show |
| Series | [review] libc: Don't use a custom wrapper macro around __has_include. | expand |
Florian Weimer has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/697
......................................................................
Patch Set 1:
(4 comments)
Thanks for the patch. The substance of the patch is okay. I would prefer if you could repost it with the minor issues mentioned below addressed, then I can push it on your behalf.
| --- /dev/null
| +++ /COMMIT_MSG
| @@ -1,0 +8,17 @@ libc: Don't use a custom wrapper macro around __has_include.
| +
| +This causes issues when using clang with -frewrite-includes to e.g., submit the
| +translation unit to a distributed compiler.
| +
| +In my case, I was building Firefox using sccache.
| +
| +See https://bugs.llvm.org/show_bug.cgi?id=43982 for a reduced test-case since I
| +initially thought this was a clang bug.
| +
| +Apparently doing this is invalid C++ per:
| +
| + * http://eel.is/c++draft/cpp.cond#7.sentence-2
PS1, Line 19:
I think it's probably better to quote the actual language in the
standard.
| +
| +(See https://bugs.llvm.org/show_bug.cgi?id=37990)
| +
| +Signed-off-by: Emilio Cobos Álvarez <emilio@crisal.io>
PS1, Line 23:
We do not use Signed-off-by, but copyright assignments, but we have
been advised that this is not necessary for changes of this size.
| +Change-Id: Id4b8ee19176a9e4624b533087ba870c418f27e60
| --- sysdeps/unix/sysv/linux/bits/statx.h
| +++ sysdeps/unix/sysv/linux/bits/statx.h
| @@ -26,12 +26,14 @@ /* Use the Linux kernel header if available. */
|
| /* Use "" to work around incorrect macro expansion of the
| __has_include argument (GCC PR 80005). */
| -#if __glibc_has_include ("linux/stat.h")
| -# include "linux/stat.h"
| -# ifdef STATX_TYPE
| -# define __statx_timestamp_defined 1
| -# define __statx_defined 1
| -# endif
| +#ifdef __has_include
PS1, Line 29:
I think the indentation is slightly off here. Each nested conditional
should add one space after the #.
| +# if __has_include("linux/stat.h")
PS1, Line 30:
Please include a space after __has_include.
| +# include "linux/stat.h"
| +# ifdef STATX_TYPE
| +# define __statx_timestamp_defined 1
| +# define __statx_defined 1
| +# endif
| +# endif
| #endif
|
| #include <bits/statx-generic.h>
* Florian Weimer: > Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/697 > ...................................................................... > > libc: Don't use a custom wrapper macro around __has_include. > > This causes issues when using clang with -frewrite-includes to e.g., submit the > translation unit to a distributed compiler. > > In my case, I was building Firefox using sccache. > > See https://bugs.llvm.org/show_bug.cgi?id=43982 for a reduced test-case since I > initially thought this was a clang bug. > > Apparently doing this is invalid C++ per: > > * http://eel.is/c++draft/cpp.cond#7.sentence-2 > > (See https://bugs.llvm.org/show_bug.cgi?id=37990) > > Signed-off-by: Emilio Cobos Álvarez <emilio@crisal.io> > Change-Id: Id4b8ee19176a9e4624b533087ba870c418f27e60 Just to be clear here, the patch is from Emilio Cobos Álvarez <emilio@crisal.io>. This is correctly recorded in Gerrit, but not reflected in the email notification. Florian
Emilio Cobos Álvarez has posted comments on this change. Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/697 ...................................................................... Patch Set 2: Managed to follow-up on Gerrit :)
Florian Weimer has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/697
......................................................................
Patch Set 2:
(5 comments)
| --- /dev/null
| +++ /COMMIT_MSG
| @@ -1,0 +8,17 @@ libc: Don't use a custom wrapper macro around __has_include.
| +
| +This causes issues when using clang with -frewrite-includes to e.g., submit the
| +translation unit to a distributed compiler.
| +
| +In my case, I was building Firefox using sccache.
| +
| +See https://bugs.llvm.org/show_bug.cgi?id=43982 for a reduced test-case since I
| +initially thought this was a clang bug.
| +
| +Apparently doing this is invalid C++ per:
| +
| + * http://eel.is/c++draft/cpp.cond#7.sentence-2
PS1, Line 19:
Done
| +
| +(See https://bugs.llvm.org/show_bug.cgi?id=37990)
| +
| +Signed-off-by: Emilio Cobos Álvarez <emilio@crisal.io>
PS1, Line 23:
Done
| +Change-Id: Id4b8ee19176a9e4624b533087ba870c418f27e60
| --- sysdeps/unix/sysv/linux/bits/statx.h
| +++ sysdeps/unix/sysv/linux/bits/statx.h
| @@ -26,12 +26,14 @@ /* Use the Linux kernel header if available. */
|
| /* Use "" to work around incorrect macro expansion of the
| __has_include argument (GCC PR 80005). */
| -#if __glibc_has_include ("linux/stat.h")
| -# include "linux/stat.h"
| -# ifdef STATX_TYPE
| -# define __statx_timestamp_defined 1
| -# define __statx_defined 1
| -# endif
| +#ifdef __has_include
PS1, Line 29:
Done
| +# if __has_include("linux/stat.h")
PS1, Line 30:
Done
| +# include "linux/stat.h"
| +# ifdef STATX_TYPE
| +# define __statx_timestamp_defined 1
| +# define __statx_defined 1
| +# endif
| +# endif
| #endif
|
| #include <bits/statx-generic.h>
| --- /dev/null
| +++ /COMMIT_MSG
| @@ -1,0 +1,16 @@
| +Parent: fcb04b9a (Introduce DL_LOOKUP_FOR_RELOCATE flag for _dl_lookup_symbol_x)
| +Author: Emilio Cobos Álvarez <emilio@crisal.io>
| +AuthorDate: 2019-11-12 19:18:32 +0100
| +Commit: Emilio Cobos Álvarez <emilio@crisal.io>
| +CommitDate: 2019-11-21 14:43:52 +0100
| +
| +libc: Don't use a custom wrapper macro around __has_include.
PS2, Line 7:
Sorry, missed this before.
Please include a reference to the bug, as (bug 25189) or [BZ #25189].
And the libc: prefix probably is not needed.
The rest looks good to me.
| +
| +This causes issues when using clang with -frewrite-includes to e.g., submit the
| +translation unit to a distributed compiler.
| +
| +In my case, I was building Firefox using sccache.
| +
| +See [1] for a reduced test-case since I initially thought this was a clang bug,
| +and [2] for more context.
| +
Emilio Cobos Álvarez has posted comments on this change. Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/697 ...................................................................... Patch Set 3: (1 comment) | --- /dev/null | +++ /COMMIT_MSG | @@ -1,0 +1,16 @@ | +Parent: fcb04b9a (Introduce DL_LOOKUP_FOR_RELOCATE flag for _dl_lookup_symbol_x) | +Author: Emilio Cobos Álvarez <emilio@crisal.io> | +AuthorDate: 2019-11-12 19:18:32 +0100 | +Commit: Emilio Cobos Álvarez <emilio@crisal.io> | +CommitDate: 2019-11-21 14:43:52 +0100 | + | +libc: Don't use a custom wrapper macro around __has_include. PS2, Line 7: Done | + | +This causes issues when using clang with -frewrite-includes to e.g., submit the | +translation unit to a distributed compiler. | + | +In my case, I was building Firefox using sccache. | + | +See [1] for a reduced test-case since I initially thought this was a clang bug, | +and [2] for more context. | +
Florian Weimer has posted comments on this change. Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/697 ...................................................................... Patch Set 3: (1 comment) | --- /dev/null | +++ /COMMIT_MSG | @@ -1,0 +1,23 @@ | +Parent: fcb04b9a (Introduce DL_LOOKUP_FOR_RELOCATE flag for _dl_lookup_symbol_x) | +Author: Emilio Cobos Álvarez <emilio@crisal.io> | +AuthorDate: 2019-11-12 19:18:32 +0100 | +Commit: Emilio Cobos Álvarez <emilio@crisal.io> | +CommitDate: 2019-11-21 10:32:48 -0500 | + | +Don't use a custom wrapper macro around __has_include (bug 25189). | + | +This causes issues when using clang with -frewrite-includes to e.g., submit the | +translation unit to a distributed compiler. | + | +In my case, I was building Firefox using sccache. | + | +See [1] for a reduced test-case since I initially thought this was a clang bug, PS3, Line 14: Sorry, I'm new to Gerrit and missed that these two lines are very long. Would you please break them around 72 characters or thereabouts? Thanks. | +and [2] for more context. | + | +Apparently doing this is invalid C++ per [cpp.cond], which mentions [3]: | + | +> The #ifdef and #ifndef directives, and the defined conditional inclusion | +> operator, shall treat __has_include and __has_cpp_attribute as if they | +> were the names of defined macros. The identifiers __has_include and | +> __has_cpp_attribute shall not appear in any context not mentioned in this | +> subclause.
Emilio Cobos Álvarez has posted comments on this change. Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/697 ...................................................................... Patch Set 4: (1 comment) | --- /dev/null | +++ /COMMIT_MSG | @@ -1,0 +1,23 @@ | +Parent: fcb04b9a (Introduce DL_LOOKUP_FOR_RELOCATE flag for _dl_lookup_symbol_x) | +Author: Emilio Cobos Álvarez <emilio@crisal.io> | +AuthorDate: 2019-11-12 19:18:32 +0100 | +Commit: Emilio Cobos Álvarez <emilio@crisal.io> | +CommitDate: 2019-11-21 10:32:48 -0500 | + | +Don't use a custom wrapper macro around __has_include (bug 25189). | + | +This causes issues when using clang with -frewrite-includes to e.g., submit the | +translation unit to a distributed compiler. | + | +In my case, I was building Firefox using sccache. | + | +See [1] for a reduced test-case since I initially thought this was a clang bug, PS3, Line 14: Done. Sorry, my fault, was using 80. Should be fine now :) | +and [2] for more context. | + | +Apparently doing this is invalid C++ per [cpp.cond], which mentions [3]: | + | +> The #ifdef and #ifndef directives, and the defined conditional inclusion | +> operator, shall treat __has_include and __has_cpp_attribute as if they | +> were the names of defined macros. The identifiers __has_include and | +> __has_cpp_attribute shall not appear in any context not mentioned in this | +> subclause.
Florian Weimer has posted comments on this change. Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/697 ...................................................................... Patch Set 4: Code-Review+2 Perfect! Thanks a lot.
diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h index abcb0d5..467dbd9 100644 --- a/misc/sys/cdefs.h +++ b/misc/sys/cdefs.h @@ -412,14 +412,6 @@ # define __glibc_has_attribute(attr) 0 #endif -#ifdef __has_include -/* Do not use a function-like macro, so that __has_include can inhibit - macro expansion. */ -# define __glibc_has_include __has_include -#else -# define __glibc_has_include(header) 0 -#endif - #if (!defined _Noreturn \ && (defined __STDC_VERSION__ ? __STDC_VERSION__ : 0) < 201112 \ && !__GNUC_PREREQ (4,7)) diff --git a/sysdeps/unix/sysv/linux/bits/statx.h b/sysdeps/unix/sysv/linux/bits/statx.h index ff3f2e8..14d5992 100644 --- a/sysdeps/unix/sysv/linux/bits/statx.h +++ b/sysdeps/unix/sysv/linux/bits/statx.h @@ -26,12 +26,14 @@ /* Use "" to work around incorrect macro expansion of the __has_include argument (GCC PR 80005). */ -#if __glibc_has_include ("linux/stat.h") -# include "linux/stat.h" -# ifdef STATX_TYPE -# define __statx_timestamp_defined 1 -# define __statx_defined 1 -# endif +#ifdef __has_include +# if __has_include("linux/stat.h") +# include "linux/stat.h" +# ifdef STATX_TYPE +# define __statx_timestamp_defined 1 +# define __statx_defined 1 +# endif +# endif #endif #include <bits/statx-generic.h>
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/697 ...................................................................... libc: Don't use a custom wrapper macro around __has_include. This causes issues when using clang with -frewrite-includes to e.g., submit the translation unit to a distributed compiler. In my case, I was building Firefox using sccache. See https://bugs.llvm.org/show_bug.cgi?id=43982 for a reduced test-case since I initially thought this was a clang bug. Apparently doing this is invalid C++ per: * http://eel.is/c++draft/cpp.cond#7.sentence-2 (See https://bugs.llvm.org/show_bug.cgi?id=37990) Signed-off-by: Emilio Cobos Álvarez <emilio@crisal.io> Change-Id: Id4b8ee19176a9e4624b533087ba870c418f27e60 --- M misc/sys/cdefs.h M sysdeps/unix/sysv/linux/bits/statx.h 2 files changed, 8 insertions(+), 14 deletions(-)