This is an archive of the discontinued LLVM Phabricator instance.

[clang][Headers] Avoid compiler warnings in builtin headers
ClosedPublic

Authored by ddcc on Jul 29 2022, 2:38 PM.

Details

Summary

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.

Diff Detail

Event Timeline

ddcc created this revision.Jul 29 2022, 2:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 29 2022, 2:38 PM
ddcc requested review of this revision.Jul 29 2022, 2:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 29 2022, 2:38 PM

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.

550

Why does this usage not need to be changed?

ddcc updated this revision to Diff 449118.Aug 1 2022, 1:56 PM

Undef macros instead, handle other header files

tra added a subscriber: tra.Aug 1 2022, 4:17 PM
tra added inline comments.
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
ddcc added inline comments.Aug 2 2022, 1:11 PM
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.

ddcc updated this revision to Diff 449403.Aug 2 2022, 1:25 PM

Error out on undef __cplusplus in CUDA headers

ddcc added a reviewer: tra.Aug 2 2022, 1:25 PM
tra added a subscriber: yaxunl.Aug 2 2022, 1:47 PM
tra added inline comments.
clang/lib/Headers/__clang_cuda_builtin_vars.h
14 ↗(On Diff #449403)

Nit: the error message sounds a bit odd.
Perhaps rephrase it as This CUDA header requires C++.

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.

ddcc added inline comments.Aug 2 2022, 3:20 PM
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.

ddcc updated this revision to Diff 449458.Aug 2 2022, 3:21 PM

Drop changes to CUDA/HIP/OpenMP headers

iana added a subscriber: iana.Aug 2 2022, 4:49 PM
iana added inline comments.
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?

iana added a reviewer: iana.Aug 2 2022, 4:49 PM
iana added inline comments.Aug 2 2022, 4:57 PM
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)

ddcc added inline comments.Aug 2 2022, 9:26 PM
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.

aaron.ballman accepted this revision.Aug 3 2022, 11:32 AM

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.
This revision is now accepted and ready to land.Aug 3 2022, 11:32 AM
This revision was landed with ongoing or failed builds.Aug 3 2022, 5:56 PM
This revision was automatically updated to reflect the committed changes.