While debugging module support using -Wsystem-headers, we discovered that if
-Werror, and -Wundef or -Wmacro-redefined are specified, they can cause errors
to be generated in these builtin headers.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Thanks for catching this! As far as these changes go, they're pretty reasonable, but don't seem like they hit all of the problem areas. There's 20+ occurrences of #if __STDC_VERSION__ and 10+ of #if __cplusplus in lib/Headers; shouldn't they all be updated?
clang/lib/Headers/stdint.h | ||
---|---|---|
94–95 | It seems to me that a cleaner approach to this is to #undef the macros we expect to be redefining before we redefine them instead of disabling the definition warning entirely. For example, it would still be useful for a user to find out that our stdint.h redefines some symbol from their own header files from an earlier inclusion. | |
551 | Why does this usage not need to be changed? |
clang/lib/Headers/__clang_cuda_builtin_vars.h | ||
---|---|---|
37 ↗ | (On Diff #449118) | Are there actual use cases where CUDA headers are used from a non-C++ compilation? I do not think they will compile with non-C++ compilation at all. I'm curious how we managed to run into a situation where CUDA headers may be used w/o C++. I think a better fix here and other CUDA headers may be to add #if !defined(__cplusplus) #error CUDA headers must be used from a CUDA/C++ compilation. #endif |
clang/lib/Headers/__clang_cuda_builtin_vars.h | ||
---|---|---|
37 ↗ | (On Diff #449118) | Sure, I can do that. I'm not familiar with CUDA or its headers, so I previously just updated them all. |
clang/lib/Headers/__clang_cuda_builtin_vars.h | ||
---|---|---|
14 ↗ | (On Diff #449403) | Nit: the error message sounds a bit odd. |
clang/lib/Headers/__clang_hip_cmath.h | ||
381 ↗ | (On Diff #449403) | HIP headers are also not expected to compile w/o C++ and we may want a #errorhere, too. Actually, I'm not even sure if we shopuld/need to add #if defined(__cplusplus)in files that are clearly C++ -- undefined macro warning will be the least of one's worries if they attempt to compile it w/o C++. Perhaps it would make sense to limit these changes to the headers intended to be used from both C and C++? Both CUDA and HIP are expected to never be included from a non-C++ compilation and will never trigger the undefined macro warning on __cplusplus. @yaxunl - FYI. |
clang/lib/Headers/__clang_hip_cmath.h | ||
---|---|---|
381 ↗ | (On Diff #449403) | Sure, I'm fine with leaving those headers alone since I'm not familiar with them. |
clang/lib/Headers/stdint.h | ||
---|---|---|
99–100 | What are you seeing that's defining __int_least64_t and all these other ones by the time you get here? It's surprising to me that so much preprocessor state exists when you hit this point considering that this file doesn't include anything else. Is this some kind of artifact with how the OS module map is making a module for stdint.h? |
clang/lib/Headers/stdint.h | ||
---|---|---|
99–100 | Oh I see, it's not these ones that are the problem, it's the redefinitions below. I guess it's a bigger change, but I wonder if flipping the order would be cleaner? e.g. #ifdef __INT32_TYPE__ # define __int_least32_t int32_t #endif #ifdef __INT64_TYPE__ # ifndef __int_least32_t # define __int_least32_t int64_t # endif #endif I guess it's an extra line per type doing it that way, but maybe it's more obvious? (Maybe I just don't do a lot of preprocessor programming, but #undef feels like a code smell to me) |
clang/lib/Headers/stdint.h | ||
---|---|---|
99–100 | I'm a little reluctant to change this file too much since it's tedious and error-prone, but yeah if we can agree on a solution, I can go ahead and make the change. |
These changes look reasonable to me, thanks!
clang/lib/Headers/stdint.h | ||
---|---|---|
99–100 | I'm not toooooo sad about the #undef though this does make preprocessing this file do more work. However, rearranging the macros in this file might be a challenge given how much these macros interact with one another. From the comments just above: * These macros are defined using the same successive-shrinking approach as * the type definitions above. It is likewise important that macros are defined * in order of decending width. |
It seems to me that a cleaner approach to this is to #undef the macros we expect to be redefining before we redefine them instead of disabling the definition warning entirely. For example, it would still be useful for a user to find out that our stdint.h redefines some symbol from their own header files from an earlier inclusion.