Page MenuHomePhabricator

Add support for GCC's '__auto_type' extension.
ClosedPublic

Authored by comex on Sep 8 2015, 12:44 AM.

Details

Reviewers
rsmith
Summary

Add support for GCC's '__auto_type' extension.

As per the GCC manual: https://gcc.gnu.org/onlinedocs/gcc/Typeof.html

Implemented in GCC 4.9, __auto_type is similar to C++11 auto but works in C.
The most compelling use case is for macros; the above manual page explains the
need pretty well, so I won't repeat it here.

This implementation differs from GCC's in also supporting __auto_type in C++,
treating it the same as auto. I don't see any good reason not to, because
otherwise headers intended to be used from both languages can't use it (you
could use a define that expands to __auto_type or auto depending on the
language, but then C++ pre-11 is broken). However, for sanity's sake, it
prevents using __auto_type instead of auto with C++11+-specific functionality
such as decltype(auto), auto arguments to lambdas, and auto before a trailing
return type.

It also differs by allowing the following (by default, due to reusing the C++
auto behavior) which GCC rejects: using __auto_type * and the like, and
declaring multiple variables in one statement. It doesn't seem generally
harmful to allow them, although it increases the risk of accidental
incompatibility.

Diff Detail

Event Timeline

comex updated this revision to Diff 34194.Sep 8 2015, 12:44 AM
comex retitled this revision from to Add support for GCC's '__auto_type' extension..
comex updated this object.
comex added a subscriber: cfe-commits.
vsk added a subscriber: vsk.Sep 11 2015, 6:16 PM

First off, welcome! And thanks for patch.

This basically LGTM, but I have one small change I'd like to see (inline comment).

Hm, the spec says "The sizeof operator shall not be applied to... an lvalue that designates a bit-field". Sounds like a bug to me.

lib/Sema/SemaType.cpp
2734

Hm, this is a little hard to read. IIUC it's also sensitive to re-ordering of enum values. I'd be happier if you introduced a switch here, and made the LHS enum type explicit.

test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.spec.auto/p3-1y.cpp
4

Do you know what this FIXME alludes to? Seems rather mysterious to me.

rsmith added a subscriber: rsmith.Sep 11 2015, 6:46 PM
rsmith added inline comments.
include/clang/AST/Type.h
1214

Please use enum class here; we don't use enum struct anywhere in Clang or LLVM.

1220

Maybe GNUAutoType?

3949–3950

auto and __auto_type should probably not profile differently; they mangle the same, so this will lead to mangling collisions. (It's not completely clear that we need to retain the distinction between auto and __auto_type here at all, since they have identical semantics, but it's probably a good thing for source fidelity. We don't maintain a difference between bool and _Bool, for what it's worth.)

lib/Sema/SemaExpr.cpp
352

const AutoType* AT -> const AutoType *AT`. Also, remove the unnecessary .getTypePtr() here.

test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.spec.auto/p3-1y.cpp
4

It says that the relevant wording in the standard got moved to a different paragraph number (due to the insertion of more rules beforehand for deduced return types and generic lambdas).

comex updated this revision to Diff 34787.Sep 14 2015, 11:55 PM
comex updated this object.

Per vsk:

  • Changed to an explicit switch, and changed ContainsPlaceholderType logic.

Note: Currently, GetDeclSpecTypeForDeclarator determines ContainsPlaceholderType at two different levels depending on the context: usually it calls containsPlaceholderType on the DeclSpec, but for conversion methods it searches the Type itself using getContainedAutoType. This confused me, because I can't come up with any conversion method declaration that would actually yield different answers for those predicates, given the definition of getContainedAutoType - at least that wouldn't generate an error elsewhere. (They're different in the case of an auto lambda parameter, in which case the AutoType has already been converted to a dependent template type, but that's not a conversion.) The old patch implicitly assumed containsPlaceholderType was true when it checked getTypeSpecType; I could avoid this by determining which auto keyword is being used a different way in the conversion case, but since that would be really ugly, I went ahead and removed ContainsPlaceholderType in favor of always calling containsPlaceholderType, effectively partially reverting SVN revision 181108. This might be wrong.

For example, these all still properly fail with this patch:

template <typename T> struct F {};
struct A {
  operator auto() {}
  operator auto*() {}
  operator auto*() {}
  operator F<auto>() {}
  operator auto() -> auto {}
  operator __typeof__(auto(*)())() {}
  operator __typeof__([](auto){})() {} // bad error though (unaffected by patch)
};

I did come up with this snippet which trips an assertion in stock Clang (also unaffected by this patch):

typedef __typeof__(auto()) *T;
T lol = (T) 2;

...and here's another one:

typedef &decltype(auto)::x T;

I guess I should report these separately.

  • Added error for using __auto_type with a bitfield in C; since it was easy, I also added the same for typeof, despite it not being strictly related.

Per rsmith:

  • Changed enum struct to enum class, and AutoTypeExt to GNUAutoType.
  • As far as I understand, having distinguishable values profile the same would break uniquing - in this case, that done in getAutoType(). In theory, mangling shouldn't be a problem because the cases where 'auto' ends up being mangled shouldn't be allowed with __auto_type: AFAICT only C++14 (template) functions with deduced return types. But it turns out I wasn't properly banning those, which I fixed. I also added an assertion to the two manglers that __auto_type isn't encountered.
  • Fixed whitespace and removed getTypePtr().

Also:

  • Improved error message with code like __auto_type x = {2, 3}; in C (previously it referred to std::initializer_list)
vsk added a comment.Oct 12 2015, 11:43 AM

This lgtm. If you don't have commit rights, I can land this for you if you'd like.

rsmith added inline comments.Oct 12 2015, 12:06 PM
include/clang/Basic/DiagnosticSemaKinds.td
1734

pass -> use

Also, why not? Just because GCC messes this up, doesn't mean we have to.

lib/AST/ItaniumMangle.cpp
2657–2658

Why not?

template<typename T> void f(decltype(new __auto_type(T())));

... would need a mangling, right? (Or do you prohibit __auto_type there?)

lib/Parse/ParseDeclCXX.cpp
1119 ↗(On Diff #34787)

That would be ill-formed; revert this change.

lib/Sema/SemaExpr.cpp
352

This has not been addressed.

lib/Sema/SemaType.cpp
1480

Should we really allow using __auto_type to introduce a generic lambda? It seems like there's a major open design question here: either we should allow __auto_type only in GCC-compatible contexts (that is, as a decl-specifier that's not a function return type), or we should allow it everywhere we allow auto and make it a synonym for auto in C++ (in which case it needs to be mangled, and the distinction between auto and __auto_type should probably not affect the canonical type).

2672–2675

Do you really want to allow __auto_type here? This is inconsistent with what you do for return types.

comex marked 5 inline comments as done.Oct 12 2015, 5:27 PM
comex added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
1734

By analogy with the standards-defined inability to use sizeof on bit fields, it made sense to me to do what GCC does and forbid getting their type in other ways. Though I suppose the restriction is somewhat questionable in the first place.

lib/AST/ItaniumMangle.cpp
2657–2658

Since my goal is to only allow __auto_type in C-compatible contexts, this should be prohibited, but wasn't. Fixed. (Any other cases I haven't thought of?)

lib/Parse/ParseDeclCXX.cpp
1119 ↗(On Diff #34787)

Fixed. Durr.

lib/Sema/SemaType.cpp
1480

My goal was to do the former; if you'd prefer to just make it a synonym. In this case, the patch prevents __auto_type from being used in lambda parameters elsewhere.

comex updated this revision to Diff 37202.Oct 12 2015, 5:45 PM
comex updated this object.
comex marked 3 inline comments as done.

Fixed raised issues.

(I don't have commit rights.)

Thanks, this essentially looks good to me. I can't think of any other cases where C++ allows auto that you've not covered.

lib/Sema/SemaExprCXX.cpp
1172–1173 ↗(On Diff #37202)

How about instead handling this...

lib/Sema/SemaType.cpp
2685–2690

... here.

thakis added inline comments.
test/SemaCXX/auto-type-from-cxx.cpp
15

Shouldn't this say "warning: __auto_type is a gnu extension" (since this uses -std=c++14, not -std=gnu++14)?

comex added inline comments.Oct 13 2015, 1:10 AM
lib/Sema/SemaType.cpp
2672–2675

Also a mistake.

test/SemaCXX/auto-type-from-cxx.cpp
15

Hmm... when I added ext_auto_type to the .td, I didn't notice the difference between Extension and ExtWarn. Since the patch currently uses Extension, it won't warn by default, but will with -pedantic, -Wgnu, or -Wgnu-auto-type. The C test uses -pedantic so it gets the warning.

Is there an explicit policy on which extensions should be ExtWarn? Looking at the rest of that file, ExtWarns seem to be mostly either (a) standardized extensions (which will warn if -std is too early) and (b) 'extensions' that are more like "Clang will accept your buggy code" than features. Most GNU extensions are just Extension, so I think it makes sense to do the same for __auto_type.

I could update the C++ test to add -pedantic, but arguably it makes more sense to test the fact that the warning is not emitted by default.

comex added inline comments.Oct 13 2015, 1:12 AM
lib/Sema/SemaType.cpp
2672–2676

(Clarification: The "also a mistake" comment was meant to be submitted earlier, but was left in the Phabricator draft state; I already fixed this.)

thakis added inline comments.Oct 13 2015, 8:41 AM
test/SemaCXX/auto-type-from-cxx.cpp
15

I'm not sure either. typeof warns with -std=c++14 but not with -std=gnu++14. From a user perspective, this makes sense to me: I want to write standard c++ and I want the compiler to help me with that, but I don't want to get all the pedantic warnings that -pedantic entails. For example, consider using clang-cl to build Windows code, and wanting MSVC to be able to build said Windows code too. (This can of course go wrong with standard C++ too, but in that case I'm running into MSVC bugs which will eventually be fixed.) So if it's possible to match how typeof works, I think that'd be good. If it's not possible, this wouldn't be the only GNU extension allowed in -std=c++14 mode though, so it's not a big thing.

comex added inline comments.Oct 13 2015, 12:14 PM
test/SemaCXX/auto-type-from-cxx.cpp
15

As far as I can tell, typeof doesn't warn at all. In standard mode it's an *error* because typeof is treated as a normal identifier (so that code that uses it as such doesn't break), but if you use the reserved-namespace keyword __typeof, there is no warning even with -Weverything.

thakis added inline comments.Oct 13 2015, 12:51 PM
test/SemaCXX/auto-type-from-cxx.cpp
15

Ah, ok. That's ok as-is then. It'd be nice if there was something that warning on GNU extensions, but that's independent of this patch :-)

One more ping. As far as I know, everything has been addressed.

(Ping? I verified yesterday that the patch still applies and passes regression tests.)

rsmith added a comment.Nov 9 2015, 1:09 PM

Sorry for the long delay. This looks to be in good shape.

include/clang/AST/Type.h
1214

The explicit underlying type here doesn't seem necessary.

1442

Use unsigned not AutoTypeKeyword. MSVC struct layout inserts extra padding if your bit-fields don't all have the same type.

include/clang/Basic/DiagnosticSemaKinds.td
1681–1682

How about "in type allocated by new" or similar? The type operand isn't really an argument.

lib/Sema/SemaTemplateDeduction.cpp
4012–4014

Move this bailout to before we build the template parameter and substitute it into the type.

4016

Should be DAR_FailedAlreadyDiagnosed so the caller doesn't diagnose it again.

lib/Sema/SemaType.cpp
1480

Can you move the auto_type case label lower to make this more obvious? (Put a break into the if and replace the else with fallthrough to avoid putting the label inside the else.)

1517

The /*Keyword*/ comment here doesn't add anything, thanks to your enumeration. Remove it and clang-format this line?

2619–2620

Does this still do the right thing for the IK_ConversionFunctionId case? It doesn't look like it will: in that case, the auto is in the declarator-id, not in the decl-specifier-seq.

2623–2624

This is now reachable.

3767–3771

Please deal with this in the "checking for auto" block at the top rather than repeating the diagnostic code here with magic numbers; maybe change

(!SemaRef.getLangOpts().CPlusPlus11 || !D.isFunctionDeclarator())) {

to

(!SemaRef.getLangOpts().CPlusPlus11 || !D.isFunctionDeclarator() || D.getDeclSpec().getTypeSpecType() == DeclSpec::TST_auto_type)) {

up there.

rsmith added inline comments.Nov 9 2015, 1:10 PM
lib/Sema/SemaType.cpp
2619–2620

Disregard this comment; I tried to delete it but it looks like phabricator submitted it anyway.

comex updated this revision to Diff 39838.Nov 10 2015, 11:25 AM
comex marked 16 inline comments as done.

Addressed comments. Thanks, rsmith!

lib/Sema/SemaType.cpp
3767–3771

For the sake of clarity, I removed that whole bit from the if condition instead and moved the C++11 test down to where it sets Error. Two bits of fallout from this:

  • typedef decltype(auto) f(); gets diagnosed there rather than later with a different error; I updated the test, since both errors seem reasonable.
  • A bug is fixed where definitions like auto f() { return 5; } were not being caught with -std=c++14 -Wc++98-compat. I updated the c++98-compat test to add this case.
rsmith accepted this revision.Nov 10 2015, 1:19 PM
rsmith added a reviewer: rsmith.

This LGTM, do you have SVN access or would you like someone to commit for you?

lib/Sema/SemaType.cpp
2616–2618

This comment is out of date. Delete these three lines?

3767–3771

Does this change also fix PR25449?

test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.spec.auto/p7-1y.cpp
52

Thanks, this new diagnostic is a lot better :)

This revision is now accepted and ready to land.Nov 10 2015, 1:19 PM

I don't have SVN access. If you want to commit it, feel free to remove that comment, or else I'll update the diff tomorrow.

I just tried the test case in 25449 and it now gives a proper error rather than crashing. (Though when trying to reduce a test failure I was encountering, I did find yet another pre-existing crasher, which doesn't seem to be related to the code being changed here. I should definitely check which of the crashers I found have existing bug reports and file the rest.)

Committed as r252690.

Could I tempt you to put together a documentation patch for this, and a
change for the release notes?