Page MenuHomePhabricator

Add -fgnuc-version= to control __GNUC__ and other GCC macros
ClosedPublic

Authored by rnk on Sep 25 2019, 3:00 PM.

Details

Summary

I noticed that compiling on Windows with -fno-ms-compatibility had the
side effect of defining GNUC, along with EXCEPTIONS, GXX_RTTI,
and a number of other macros for GCC compatibility. This is undesirable
and causes Chromium to do things like mix
attribute__ and __declspec,
which doesn't work. We should have a positive language option to enable
GCC compatibility features so that we can experiment with
-fno-ms-compatibility on Windows. I propose that -fgnuc-version= can be
that option.

My issue aside, users have, for a long time, reported that GNUC
doesn't match their expectations in one way or another. We have
encouraged users to migrate code away from this macro, but new code
continues to be written assuming a GCC-only environment. There's really
nothing we can do to stop that. By adding this flag, we can allow them
to choose their own adventure with GNUC.

This overlaps a bit with the "GNUMode" language option from -std=gnu*.
I'm not sure how to reconcile them. The gnu language mode tends to
enable non-conforming behaviors that we'd rather not enable by default,
but these feature macros, like __EXCEPTIONS, are pretty benign.

Helps address PR42817

Diff Detail

Event Timeline

rnk created this revision.Sep 25 2019, 3:00 PM
hans accepted this revision.Sep 26 2019, 6:55 AM

This looks reasonable to me. Might be worth mentioning in ReleaseNotes too :-)

This revision is now accepted and ready to land.Sep 26 2019, 6:55 AM

This is great, thanks!

Shouldn't __GNUG__ match __GNUC__?

nickdesaulniers accepted this revision.Oct 1 2019, 10:59 PM

The __GNUG__ macro is defined to be 4 rather than matching __GNUC__

MaskRay accepted this revision.Oct 1 2019, 11:16 PM
MaskRay added a subscriber: MaskRay.

This will make it much easier to test clang's compatibility with GCC, especially in projects that heavily use __GNUC__ or its friends, e.g. glibc :)

rsmith added a comment.Oct 7 2019, 5:32 PM

From a big-picture perspective, I would like us to move to treating MS and GNU extensions in the same way (at the cc1 level) to the extent that we can. I think this moves us in that direction.

One comment, though: I'm not convinced that the __EXCEPTIONS macro should be controlled by this flag. Many compilers define this when exceptions are available (ARM, IBM, HP, EDG in all modes -- including MSVC-compatible mode, ...) and it's not obvious to me whether it is in fact a GNU extension or has more history than that. (It's also a somewhat-documented Clang feature independent of GCC compatibility.)

Do you know why we turn that macro off under MSVCCompat? If EDG can get away with defining it in MSVC-compatible mode, I would expect that we can too.

rnk added a comment.Oct 8 2019, 4:57 PM

From a big-picture perspective, I would like us to move to treating MS and GNU extensions in the same way (at the cc1 level) to the extent that we can. I think this moves us in that direction.

Sounds good.

One comment, though: I'm not convinced that the __EXCEPTIONS macro should be controlled by this flag. Many compilers define this when exceptions are available (ARM, IBM, HP, EDG in all modes -- including MSVC-compatible mode, ...) and it's not obvious to me whether it is in fact a GNU extension or has more history than that. (It's also a somewhat-documented Clang feature independent of GCC compatibility.)

Do you know why we turn that macro off under MSVCCompat? If EDG can get away with defining it in MSVC-compatible mode, I would expect that we can too.

I doubt there's any good reason for it. I'll remove that change from this patch and treat it separately. I think we can simply define it in all modes that enable exceptions.

Shouldn't __GNUG__ match __GNUC__?

+1

rnk updated this revision to Diff 224141.Oct 9 2019, 1:26 PM
  • add docs, release note
  • keep __EXCEPTIONS if !ms
  • keep private_extern if !ms
  • set GNUG value with flag
programmerjake requested changes to this revision.Oct 9 2019, 1:38 PM
programmerjake added inline comments.
clang/lib/Frontend/InitPreprocessor.cpp
590

Pretty sure that should be GNUCMajor

This revision now requires changes to proceed.Oct 9 2019, 1:38 PM
rnk updated this revision to Diff 224155.Oct 9 2019, 2:03 PM
  • Fix GNUG, tighten tests for it
programmerjake accepted this revision.Oct 9 2019, 2:08 PM
This revision is now accepted and ready to land.Oct 9 2019, 2:08 PM
This revision was automatically updated to reflect the committed changes.