Page MenuHomePhabricator

[Sema] Error out early for tags defined inside an enumeration.

Authored by vsapsai on Aug 23 2017, 5:52 PM.



This fixes PR28903 by avoiding access check for inner enum constant. We
are performing access check because one enum constant references another
and because enum is defined in CXXRecordDecl. But access check doesn't
work because FindDeclaringClass doesn't expect more than one EnumDecl
and because inner enum has access AS_none due to not being an immediate
child of a record.

The change detects an enum is defined in wrong place and allows to skip
parsing its body. Access check is skipped together with body parsing.
There was no crash in C, added test case to cover the new error.


Diff Detail


Event Timeline

vsapsai created this revision.Aug 23 2017, 5:52 PM

I have also considered adding tests for structs and unions defined inside an enumeration but decided it doesn't add much value, given the code is working for all TagDecl.

Another option I had in mind is having a note pointing to outer enum. But the nesting is done in the same file so the outer enum shouldn't be hard to find. So I decided that complexity outweighs the added value.

If anybody thinks the aforementioned changes are valuable, please tell so.

It occurred to me that another approach for fixing the crash can be setting IsTypeSpecifier to true while we ParseCastExpression. According to

  ( type-id ) cast-expression
  type-specifier-seq abstract-declarator[opt]

it looks reasonable to require IsTypeSpecifier during parsing part of cast expression inside parentheses.

What do you think? Is this approach worth pursuing? Does it sound better than the one offered initially?

Ping. If you don't have time to review the change in depth, please say if the approach is wrong on high level.

rnk edited edge metadata.Sep 13 2017, 4:46 PM

I think this looks good, even without fixing the access control crash, this seems like a diagnostic improvement.

128 ↗(On Diff #112487)

This is a simpler test case that fixes the extraneous diagnostics:

struct PR28903 {
  enum {
    E1 = (enum {  // expected-error-re {{...}}
            E2, E3 = E2
  } e;
doug.gregor accepted this revision.Sep 14 2017, 3:19 PM

Looks good to me; sorry for the delay.

This revision is now accepted and ready to land.Sep 14 2017, 3:19 PM
vsapsai updated this revision to Diff 115333.Sep 14 2017, 6:33 PM
  • Remove extraneous diagnostics in test case per Reid Kleckner feedback.
vsapsai marked an inline comment as done.Sep 14 2017, 6:36 PM
vsapsai added inline comments.
128 ↗(On Diff #112487)

Removed extraneous diagnostics in slightly different way. Keep enum constants prefixed with PR28903 because in C struct doesn't create a scope for enum constants and I want to avoid polluting test namespace with common names like E1, E2, E3.

This revision was automatically updated to reflect the committed changes.
vsapsai marked an inline comment as done.

Hi all,
this change is causing busybox to fail to compile.
The faulty enum is here:
The nested union is inside a sizeof statement.

Something like this:

enum {
  A = sizeof(union {
    int x;
    int y;

I this behavior expected ? Do you suggest to patch busybox ?

Many Thanks

Thanks for following up, Alberto. I haven't expected such a use case. It is possible to achieve the same with LSA_SIZEOF_SA = sizeof(((len_and_sockaddr *)0)->u) but I don't like it and don't want to force developers using such approach.

For solving this problem I think to restrict error only to C++ and to allow tags inside enums for C. Alberto, what do you think, will it work for you? And from implementation perspective allowing tags in enums for C should be safe because things go haywire for C++ while checking access rules and C doesn't have access rules.

Yes, restricting the error to C++ would work. Many thanks.

Submitted for review - [fixup][Sema] Allow in C to define tags inside enumerations.