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(-)