Message ID | 20191112183115.2048865-1-emilio@crisal.io |
---|---|
State | New |
Headers | show |
Series | libc: Don't use a custom wrapper macro around __has_include. | expand |
* Emilio Cobos Álvarez: > This causes issues when using clang with -frewrite-includes to e.g., submit the > translation unit to a distributed compiler. 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. I've tried to push this into Gerrit, but we can't Cc: people who do not have accounts in the tool. Not your fault. We can continue on the mailing list. > 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 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> 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. > diff --git a/sysdeps/unix/sysv/linux/bits/statx.h b/sysdeps/unix/sysv/linux/bits/statx.h > index ff3f2e8973..14d5992226 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 The indentation is slight off here, I think. Each nested conditional adds one space after the #. Please also include a space after __has_include. Thanks, Florian
On 11/21/19 2:00 PM, Florian Weimer wrote: > * Emilio Cobos Álvarez: > >> This causes issues when using clang with -frewrite-includes to e.g., submit the >> translation unit to a distributed compiler. > > 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. I think I did this right, but let me know otherwise :) > I've tried to push this into Gerrit, but we can't Cc: people who do not > have accounts in the tool. Not your fault. We can continue on the > mailing list. Ah, cool. Are there docs for pushing to Gerrit? I saw the revision, and it seems I can edit the revision description, but not sure how to set up the instance for glibc :) I am familiar with Gerrit itself so I'd be ok with following-up there if needed. Anyhow hopefully the second version of the patch looks good. >> 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 > > I think it's probably better to quote the actual language in the > standard. Rephrased the commit message to be a little cleaner and quote the standard directly. > >> (See https://bugs.llvm.org/show_bug.cgi?id=37990) >> >> Signed-off-by: Emilio Cobos Álvarez <emilio@crisal.io> > > 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. Removed. Thanks! -- Emilio >> diff --git a/sysdeps/unix/sysv/linux/bits/statx.h b/sysdeps/unix/sysv/linux/bits/statx.h >> index ff3f2e8973..14d5992226 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 > > The indentation is slight off here, I think. Each nested conditional > adds one space after the #. Please also include a space after > __has_include. > > Thanks, > Florian >
diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h index abcb0d5e3c..467dbd9547 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 ff3f2e8973..14d5992226 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>
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> --- misc/sys/cdefs.h | 8 -------- sysdeps/unix/sysv/linux/bits/statx.h | 14 ++++++++------ 2 files changed, 8 insertions(+), 14 deletions(-)