AaronBallman wrote: Maybe my brain still isn't engaging after the holidays, but I'm still not seeing why this change is correct. From the point when Clang gets involved:
* The absolute path includes glibc's `limits.h`, which includes the next header in the chain, which is Clang's: https://sourceware.org/git/?p=glibc.git;a=blob;f=include/limits.h#l122 * If `_GCC_LIMITS_H` is not defined (but it is in this case), we define it: https://github.com/llvm/llvm-project/blob/6b12272353b45def33bf5814cdf9e8587f32d40e/clang/lib/Headers/limits.h#L18 * Then, if we're hosted and there's another `limits.h`, we include it again: https://github.com/llvm/llvm-project/blob/6b12272353b45def33bf5814cdf9e8587f32d40e/clang/lib/Headers/limits.h#L24 * This goes on to find musl's header which doesn't protect against being included this way. But your changes don't impact that situation because it is avoiding the include when `_LIBC_LIMITS_H` is defined, but musl uses `_LIMITS_H`: https://elixir.bootlin.com/musl/v1.2.5/source/include/limits.h#L1 * What's more, glibc's use of `_LIBC_LIMITS_H` ends before doing the `include_next`: https://sourceware.org/git/?p=glibc.git;a=blob;f=include/limits.h#l115 So as best I can tell, this is basically another variant of the same thing we were already doing to avoid re-including (part of) glibc, which is why I don't see how that helps. I would have expected the changes to be something more along the lines of (before the block where we define `_GCC_LIMITS_H`): ``` /* When building glibc on a musl libc system, glibc will include Clang's limits.h, which will include musl's limits.h, which cannot handle being included that way. Avert this #include_next madness by skipping musl's header. */ #if defined __GNUC__ && defined _GCC_LIMITS_H_ && !defined _LIMITS_H #define _LIMITS_H #endif ``` but that leads to the question of: why is including musl's `limits.h` a problem? It redefines macros, but that is fine on its own: https://godbolt.org/z/4nGf8zzxE If glibc is expecting different values, that seems like glibc could run into the issue if musl's header was discoverable on the include paths without involving Clang. https://github.com/llvm/llvm-project/pull/120526 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
