aaron.ballman added subscribers: jdoerfert, ABataev, rnk. aaron.ballman added a comment.
In D150646#4558543 <https://reviews.llvm.org/D150646#4558543>, @aidengrossman wrote: > I'm not opposed to a revert, but I know some downstream users like MinGW have > already adapted to this change so I'm not sure how much headache it would > cause them to do a revert. > > Maybe I'm not understanding things correctly, but I originally > `_MSC_EXTENSIONS` patch as people were enabling the microsoft extensions > (thus enabling the builtin) and still including `cpuid.h` which caused an > error due to redefining a builtin function. There seems to be another issue > there where multiple flags need to be set (`-fms-extensions`, > `-fms-compatibility`, and `fms-compatibility-version`) in order to get > `_MSC_EXTENSIONS` defined while only passing `fms-extensions` gets the > builtin defined, but not the macro. I'm not sure if just passing > `-fms-extensions` is even a valid configuration, but it does error out then. > I believe that's a separate issue to the auxiliary triple issue you mentioned. Separating threads a bit: - Regarding removing the `_MSC_EXTENSIONS` check -- you're right, that was a think-o suggestion on my part, we still need that to guard the definition in the header file. Sorry for that! - Regarding when `_MSC_EXTENSIONS` is defined -- I think it's possibly a mistake that we don't define that when passing `-fms-extensions`. `-fms-extensions` is intended to control whether we support extensions to MSVC, such as use of `__declspec` attributes. And `-fms-compatibility` is intended to control whether we're trying to be compatible with MSVC rather than be compatible with GCC or the standards. Passing `-fms-compatibility` automatically adds `-fms-extensions`. I think the issue is here: https://github.com/llvm/llvm-project/blob/a6d673070963ed0900bd33963a9b62add07339f4/clang/lib/Basic/Targets/OSTargets.cpp#L265 -- we're looking for `MSVCCompat` to decide what macros to define, and only if we're in compatibility mode and enabled extensions do we enable `_MSC_EXTENSIONS` here https://github.com/llvm/llvm-project/blob/a6d673070963ed0900bd33963a9b62add07339f4/clang/lib/Basic/Targets/OSTargets.cpp#L229 -- I think it might be reasonable for us to lift the `_MSC_EXTENSIONS` logic out so it's not controlled by `MSVCCompat`, wdyt @rnk? (That's a separate concern from this patch though.) - A revert would be appreciated so that we have time to untangle the actual root cause of the problems we're seeing at Intel where the wrong set of builtins are being enabled for offload targets. That's not caused by your patch, but your patch is causing us to hit the issue when we previously hadn't been hitting it. However, if it's quicker/easier for someone to fix that problem instead of doing a revert, that'd be even better -- but I do worry about timing given that we just cut the Clang 17 branch. CC @ABataev @jdoerfert as they may have seen this problem (https://godbolt.org/z/8Yq1vzEnT) with offload targets before and have opinions/ideas as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150646/new/ https://reviews.llvm.org/D150646 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
