Page MenuHomePhabricator

Add support for -fpermissive.
AbandonedPublic

Authored by rsmith on Feb 12 2019, 3:44 PM.

Details

Reviewers
jyknight
Summary

The intent is to provide similar functionality to GCC's -fpermissive.
For Clang's approach to diagnostics, this means that we remap all
extension diagnostics to no higher than a warning. As with GCC, the
warnings can then be suppressed with -w.

Diff Detail

Event Timeline

rsmith created this revision.Feb 12 2019, 3:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 12 2019, 3:44 PM
Herald added a subscriber: jdoerfert. · View Herald Transcript

Hm -- in GCC, -fpermissive has nothing at all to do with -pedantic/-pedantic-errors, but I suppose it should be fine to do this way.

jyknight accepted this revision.Mar 4 2019, 12:48 PM
This revision is now accepted and ready to land.Mar 4 2019, 12:48 PM

I'm not super keen on supporting -fpermissive -- what is the impetus for supporting this? Is it just for GCC compatibility?

rsmith added a comment.Mar 4 2019, 2:48 PM

I'm not super keen on supporting -fpermissive -- what is the impetus for supporting this? Is it just for GCC compatibility?

Yes, exactly that -- this is to improve our drop-in compatibility with GCC. See http://llvm.org/PR40670 for an example user request for this.

In much the same way that -w can be used to mean "I just want this (crufty old, but presumed correct) code to compile; don't bother me with warnings about it", -fpermissive (and in particular -fpermissive -w) can be used to mean "I just want this (crufty, old, and relying on compiler extensions, but presumably correct) code to compile; don't bother me with complaints that it's technically ill-formed".

Hm -- in GCC, -fpermissive has nothing at all to do with -pedantic/-pedantic-errors, but I suppose it should be fine to do this way.

Yeah, I don't think it's likely that the restriction on combining those two flags will cause problems in practice.

[I don't think the behavior of -fpermissive -pedantic-errors (converting all error-by-default extensions to warnings and converting all non-error-by-default extensions to errors) was a carefully-considered design choice on GCC's part; rather, I think it's just the emergent behavior of the way they happen to deal with extension diagnostics. As a general design point for Clang, downgrading a diagnostic from error-by-default to warning-by-default shouldn't be a breaking change, but could be if we followed GCC here and someone actually used that flag combination. If someone actually hits the diagnostic for combining these two flags and can provide some kind of justification for wanting that behavior, we could reconsider.]

I'm not super keen on supporting -fpermissive -- what is the impetus for supporting this? Is it just for GCC compatibility?

Yes, exactly that -- this is to improve our drop-in compatibility with GCC. See http://llvm.org/PR40670 for an example user request for this.

I don't think we should support -fpermissive. I'm sure that we *can*, but I don't think that we *should*. My strong preference is to close that report as WONTFIX.

In much the same way that -w can be used to mean "I just want this (crufty old, but presumed correct) code to compile; don't bother me with warnings about it", -fpermissive (and in particular -fpermissive -w) can be used to mean "I just want this (crufty, old, and relying on compiler extensions, but presumably correct) code to compile; don't bother me with complaints that it's technically ill-formed".

I think -fpermssive is actively harmful to support because its use cannot be limited to old code bases to allow them to continue to limp along in a broken state. Naive users will turn it on simply because it gets their code to compile. Also, there is a big difference between -w and -fpermissive; the former is reasonable because of false positive diagnostics, but the latter simply hides utterly broken code that may or may not happen to "work". This flag is so toxic that our static analyzer won't attempt to analyze code relying on -fpermissive to compile, so there is out-of-tree cost to Clang supporting this.

As much as I usually love improving compiler compatibility, I don't think compatibility is sufficiently compelling for this feature. Personally, I think supporting -fpermissive is about as compelling as trying to be bug-for-bug compatible with another compiler (though, it is marginally better because this is at least a documented compiler flag).

The errors disabled by this feature are default-error warnings -- you can already get the same effect by using -Wno-<lots-of-things>. Why is it bad to additionally allow -fpermissive to disable them? (If we have any diagnostics which are currently default-on-warnings which should not _ever_ be disable-able, then maybe we should just fix those?)

rsmith added a comment.Mar 5 2019, 4:00 PM

The errors disabled by this feature are default-error warnings -- you can already get the same effect by using -Wno-<lots-of-things>.

Right; the -fpermissive mode added by this patch turns off strictly less than -Wno-everything. (-fpermissive still produces warnings on uses of extensions at least -- it's very similar to what -Wno-error=everything would mean if we supported it.)

The errors disabled by this feature are default-error warnings -- you can already get the same effect by using -Wno-<lots-of-things>. Why is it bad to additionally allow -fpermissive to disable them? (If we have any diagnostics which are currently default-on-warnings which should not _ever_ be disable-able, then maybe we should just fix those?)

I believe -fpermissive controls more than just default-error warnings in GCC, and that's the kind of stuff I really don't want to support. For instance, there's no warning flag for this nonsense, but GCC accepts it under -fpermissive: https://godbolt.org/z/Q-Fl0i (If you think "but who does that?", this blog post shows up on the first page of a popular search engine when I search for fpermissive: http://blog.binchen.org/posts/always-turn-on-fpermissive-for-gcc-4-6.html). Another example of -fpermissive that I've seen in the wild is: https://godbolt.org/z/-44pK5 (which is warned on in C, but not controllable via a warning flag in C++). If we're going to claim support for -fpermissive as a GCC compatibility feature, then we'll get requests to support dialects like the above examples at some point as well and that's where my concern comes in -- I don't think we should ever support that kind of broken code.

I'd be fine with what's proposed here under a different flag name though.

Ah -- I now understand your concern, and managing user expectations appropriately does seem like a potential concern.

I did not look too closely before into what exactly GCC categorized as "permerror" (errors that can be disabled with -fpermissive) vs what Clang categorized as "ExtWarn,DefaultError" diagnostics. Looking into it a little more now, I think there may not actually be substantial overlap between the two right now.

Many errors we have warning flags for are unconditional in GCC, and many that gcc allows disabling with -fpermissive are unconditional errors in clang.

You gave examples of the latter already, and e.g. here's a bunch of bogus code which you can currently build with -Wno-everything in clang, but most of which cannot be disabled in gcc (and I believe most if not all of these flags are ExtWarn, so would be disabled by this proposed -fpermissive):
https://godbolt.org/z/81NkJQ

Given that, yeah, it may not actually make sense to call this behavior -fpermissive.

I also can't really tell what the intent is behind classifying some diagnostics in Clang as ExtWarn,DefaultError and others Warning,DefaultError -- or if there even is any useful purpose there at all. I note with special-puzzlement the existence of both ext_return_missing_expr and warn_return_missing_expr.

I guess my feeling now is that perhaps we should just eliminate that categorization as meaningless, and just make -Wno-error=everything work properly (for anyone who wants to not abort the compile on broken code, but for whatever reason also loves to see build-spam so doesn't want -Wno-everything).

I also can't really tell what the intent is behind classifying some diagnostics in Clang as ExtWarn,DefaultError and others Warning,DefaultError -- or if there even is any useful purpose there at all. I note with special-puzzlement the existence of both ext_return_missing_expr and warn_return_missing_expr.

ExtWarn,DefaultError is for extensions that we disable by default. It should only ever fire on non-conforming code. Such extensions are implicitly enabled in system headers and can be explicitly enabled with a -Wno- flag.

Warning,DefaultError is for conforming code that we can accept, but that's doing something so egregious that we choose to reject it by default. Such cases should be incredibly rare. As above, such cases are permitted in system headers and the error can be downgraded via a -W flag.

ext_return_missing_expr is used in languages where a return statement with no expression is ill-formed (everything later than C90), and warn_return_missing_expr is used in the case where it's valid but is very likely to result in undefined behavior (C90). (This patch certainly has surprising behavior for these diagnostics -- demoting the former case to a warning but leaving the latter as an error -- which I think strongly suggests that this is the wrong approach for -fpermissive.)

I guess my feeling now is that perhaps we should just eliminate that categorization as meaningless, and just make -Wno-error=everything work properly (for anyone who wants to not abort the compile on broken code, but for whatever reason also loves to see build-spam so doesn't want -Wno-everything).

The existing categories seem meaningful to me. I agree that making -Wno-error=everything work would probably be a good idea.

I don't have particularly strong feelings one way or the other about accepting -fpermissive at all. For GCC compatibility, it seems like a moderately useful thing to support, but I don't think we have any interest in accepting everything that GCC accepts under -fpermissive. Perhaps the better choice is to not provide the flag at all, rather than to provide something that has the same interface but doesn't accept the same code. If not that, making -fpermissive an alias for -Wno-error=everything is probably a better approach than that of this patch.

I don't have particularly strong feelings one way or the other about accepting -fpermissive at all. For GCC compatibility, it seems like a moderately useful thing to support, but I don't think we have any interest in accepting everything that GCC accepts under -fpermissive.

Agreed that we shouldn't accept everything GCC accepts under -fpermissive.

Perhaps the better choice is to not provide the flag at all, rather than to provide something that has the same interface but doesn't accept the same code. If not that, making -fpermissive an alias for -Wno-error=everything is probably a better approach than that of this patch.

Past experience has shown that when we add a flag for compatibility purposes, users expect compatibility and when I saw the title "Add support for -fpermissive", I got very scared that we meant *all* of -fpermissive (especially when the rationale was for GCC compatibility). I think we should avoid spelling this -fpermissive.

If we go with a different name for the flag, then the user has to update their build scripts to get code to compile with Clang, which means it shouldn't be too onerous for them to spell out the specific diagnostics they need disabled (and it sort of forces them into somewhat better code hygiene by not disabling all diagnostics). I'm kind of leaning towards not providing a flag at all.

rsmith abandoned this revision.Mar 6 2019, 4:22 PM

If we go with a different name for the flag, then the user has to update their build scripts to get code to compile with Clang, which means it shouldn't be too onerous for them to spell out the specific diagnostics they need disabled (and it sort of forces them into somewhat better code hygiene by not disabling all diagnostics). I'm kind of leaning towards not providing a flag at all.

I would certainly prefer that people explicitly list the -Wno-whatever flags they're relying on when porting to Clang. There might still be some benefit in -Wno-error=everything, but it seems questionable (as James points out, users who don't enjoy build spam would likely reach for -Wno-everything instead).

In any case, I think we have consensus that this patch is the wrong direction. Abandoning.