[clang] Set begin loc on GNU attribute parsed attrs

[tbaeder]

Summary

Otherwise the source range of the passed-in ParsedAttributesWithRange
ends up being invalid and the resulting error messages rather useless.
For example:

error: fallthrough annotation in unreachable code [-Werror,-Wimplicit-fallthrough]

Instead of the more useful error message, which is produced with this
commit:

./test.c:8:5: error: fallthrough annotation in unreachable code [-Werror,-Wimplicit-fallthrough]

__attribute__((fallthrough));
^

[aaron.ballman]

Thank you for the patch! It should also have some test cases associated with it. I'd recommend adding a test to the AST directory that do AST dumps where you test the line and column information directly.

clang/lib/Parse/ParseStmt.cpp

238–239

This seems wrong -- it may not be the first attribute in the statement (this is in ParseStatementOrDeclarationAfterAttributes()), so there may already be [[]] attributes that have been parsed, which this will overwrite. You would need to see if the start range is invalid and only set this in that case.

However, wouldn't it make more sense for ParseGNUAttributes() to accept a ParsedAttributesWithRange and properly set the range there? (This may have a larger impact though, because that function gets called in a lot more code paths than this switch case does. I suspect we have a fair amount of improvements we could make with attribute source ranges.)

[tbaeder]

Updated according to Aaron's comments. I'm not sure about changing the DeclSpec member and moving ParsedAttributesWithRange to ParsedAttr.h.

Tests included.

[aaron.ballman]

I think this is a reasonable approach (including moving ParsedAttributesWithRange into Sema), but we should have test coverage for the places where we're now using a range where we weren't previously (to ensure we're okay with the positional changes we're introducing).

Adding @rsmith to ensure I've not misunderstood layering concerns.

clang/test/SemaCXX/switch-implicit-fallthrough.cpp

110–114 ↗

(On Diff #249574)

This test seems unrelated to the patch, or am I misunderstanding something?

[tbaeder]

Thanks, I'll take a look at the tests.

clang/test/SemaCXX/switch-implicit-fallthrough.cpp

110–114 ↗

(On Diff #249574)

This is the test that prompted me to write this patch - before this patch, the error message has an invalid loc. I guess I should remove this here and move it over to the source ranges tests and also test that is has the right source range.

[aaron.ballman]

clang/test/SemaCXX/switch-implicit-fallthrough.cpp

110–114 ↗

(On Diff #249574)

Ah, yeah, you should probably add this to the source range tests. If the current test here would have failed without the patch, it's also perfectly reasonable to leave this test here as well.

[tbaeder]

Sorry for taking so long but it seems like I've went down a rabbit hole a bit. My previous patch sets the range in parseGNUAttributes() unconditionally, but that seems to trigger cases such as

// FixItLoc = possible correct location for the attributes
void ProhibitAttributes(ParsedAttributesWithRange &Attrs,
                        SourceLocation FixItLoc = SourceLocation()) {
  if (Attrs.Range.isInvalid())
    return;
  DiagnoseProhibitedAttributes(Attrs.Range, FixItLoc);
  Attrs.clear();
}

in Parser.h. Because now the attributes have a valid range, clang emits lots of errors where it previously did not.

Do you have a suggestion of what to do here, should I rather go back to a more local fix for the issue?

[aaron.ballman]

In D75844#1951915, @tbaeder wrote:

Sorry for taking so long but it seems like I've went down a rabbit hole a bit. My previous patch sets the range in parseGNUAttributes() unconditionally, but that seems to trigger cases such as

// FixItLoc = possible correct location for the attributes
void ProhibitAttributes(ParsedAttributesWithRange &Attrs,
                        SourceLocation FixItLoc = SourceLocation()) {
  if (Attrs.Range.isInvalid())
    return;
  DiagnoseProhibitedAttributes(Attrs.Range, FixItLoc);
  Attrs.clear();
}

in Parser.h. Because now the attributes have a valid range, clang emits lots of errors where it previously did not.

Do you have a suggestion of what to do here, should I rather go back to a more local fix for the issue?

I think we rely on an invalid attribute range in quite a few places. IIRC, part of what makes this hard is a construct like: [[]] int i; where there is an attribute list but no actual attributes. We need to be able to prohibit the presence of an attribute list in some places (even if there are no attribute supplied). So we use the attribute range source location because looking for attribute is insufficient. (Note, this isn't specific to C++-style attributes, so it's a general mechanism we use across attribute styles).

That said, if parseGNUAttributes() only sets the range after having parsed the __attribute__ keyword, I wouldn't expect any additional diagnostics when prohibiting attributes, so it's not clear to me what's going on.

[tbaeder]

I'm looking at this again and I am not sure how it is meant to work. For example in Parser::parseClassSpecifier in ParseDeclCXX.cpp.
Here attrs is a local variable of type ParsedAttributesWithRange, already before my patch. attrs is then passed to

1. MaybeParseGNUAttributes
2. MaybeParseMicrosoftDeclSpecs
3. ParseMicrosoftInheritanceClassAttributes
4. MaybeParseCXX11Attributes

and later parseClassSpecifier calls ProhibitAttributes(attrs) a few times. ProhibitAttributes in turn will not do anything if the given attrs have an invalid (i.e. unset) range.
So, how could they ever have a valid range set? All the four functions above only take a ParsedAttributes, no range.

This is one of the cases that now (that MaybeParseGNUAttributes sets the range of the given attrs if it really parses attributes) generates errors in various test cases.
For example in clang/test/AST/ast-print-record-decl.c: File /home/tbaeder/llvm-project/clang/test/AST/ast-print-record-decl.c Line 209: an attribute list cannot appear here

Am I missing something?

[aaron.ballman]

In D75844#2076260, @tbaeder wrote:

I'm looking at this again and I am not sure how it is meant to work. For example in Parser::parseClassSpecifier in ParseDeclCXX.cpp.
Here attrs is a local variable of type ParsedAttributesWithRange, already before my patch. attrs is then passed to

1. MaybeParseGNUAttributes
2. MaybeParseMicrosoftDeclSpecs
3. ParseMicrosoftInheritanceClassAttributes
4. MaybeParseCXX11Attributes

and later parseClassSpecifier calls ProhibitAttributes(attrs) a few times. ProhibitAttributes in turn will not do anything if the given attrs have an invalid (i.e. unset) range.
So, how could they ever have a valid range set? All the four functions above only take a ParsedAttributes, no range.

This is one of the cases that now (that MaybeParseGNUAttributes sets the range of the given attrs if it really parses attributes) generates errors in various test cases.
For example in clang/test/AST/ast-print-record-decl.c: File /home/tbaeder/llvm-project/clang/test/AST/ast-print-record-decl.c Line 209: an attribute list cannot appear here

Am I missing something?

I don't think you're missing anything -- the state of the system is currently inconsistent. From (possibly faulty) memory, we previously only tracked the minimum of location information for attribute source locations and some parts of the system relied on invalid source locations to convey meaning that it shouldn't have. This is compounded by the fact that the different attribute syntaxes (gnu, C++, declspec, etc) have been worked on at different times with different assumptions about source locations, but are all generalized in the same parsed attributes data structure. Now that you're starting to more consistently track source location information across syntaxes, you're hitting some fallout from those inconsistencies. Hopefully there's not so much fallout that it cannot be handled though (if there is, please speak up and we can try to explore other options).

[tbaeder]

I know it's been a while, but here's an update on that patch.

Now that I've got https://reviews.llvm.org/D97362 and https://reviews.llvm.org/D97371 pushed, this is a much simpler change and does not break any of the existing tests anymore.

[aaron.ballman]

Thank you for picking this back up!

clang/include/clang/Parse/Parser.h

2708 ↗

(On Diff #334605)

Can we add a comment above here saying that use of this interface is discouraged and callers should use the interface that takes the attributes with range information. Eventually, we should be getting rid of the range-less parsing functionality.

2708–2714 ↗

(On Diff #334605)

We might as well take the opportunity to fix the style issues.

2720–2721 ↗

(On Diff #334605)

2730 ↗

(On Diff #334605)

A similar comment here would be handy.

2730–2734 ↗

(On Diff #334605)

Same

2739–2740 ↗

(On Diff #334605)

clang/lib/Parse/ParseDecl.cpp

165–167 ↗

(On Diff #334605)

Don't feel obligated to fix the naming style issues here -- that's quite a bit more churn. But if you did fix it, I wouldn't complain either.

clang/test/AST/sourceranges.cpp

112 ↗

(On Diff #334605)

This test looks to be failing the CI pipeline currently.

[aaron.ballman]

LGTM!

[tbaeder]

Damn. Reverted again for the time being. The libc++ build seems to fail and I won't have time to look into that this week. :(

[aaron.ballman]

In D75844#2664119, @tbaeder wrote:

Damn. Reverted again for the time being. The libc++ build seems to fail and I won't have time to look into that this week. :(

Thank you for the quick revert! From looking at the test failure, it looks like we may be relying on accepting attributes in more places than we expected.

[tbaeder]

Yep. I'll try to come up with a test and a fix next week.

[tbaeder]

A little earlier than expected. Happy Easter :P

[aaron.ballman]

clang/test/Parser/cxx0x-attributes.cpp

377 ↗

(On Diff #334954)

Can you also add a test for __declspec attributes, like template <class _Tp, class _Alloc> friend class __declspec(code_seg("whatever")) vector4;? That is accepted by MSVC: https://godbolt.org/z/b8Gc3Y4Tr

[tbaeder]

I compiled libc++ with a clang that has this patch applied and didn't run into more problems.

[aaron.ballman]

Thank you for the additional test case, this LGTM again.

[thakis]

The test fails on macOS: http://45.33.8.238/macm1/6821/step_6.txt

Please take a look and revert for now if it takes a while to fix.

[aaron.ballman]

In D75844#2668095, @thakis wrote:

The test fails on macOS: http://45.33.8.238/macm1/6821/step_6.txt

Please take a look and revert for now if it takes a while to fix.

I've committed a speculative fix in 241d42c38226e46ff01b774da3167ec54420bf24.

[brooksmoses]

FWIW, this now causes Clang to produce an error on this code, when it didn't before:

using namespace::foo __attribute__((deprecated("message")));

I discussed this with Richard Smith, who points out that GCC does not accept this and it's not permitted according to the C++ standard, so this should probably be considered an accidental fix of a longstanding bug. With that said, would it be useful to add this as a tested case?

[aaron.ballman]

In D75844#2677457, @brooksmoses wrote:

FWIW, this now causes Clang to produce an error on this code, when it didn't before:

using namespace::foo __attribute__((deprecated("message")));

I discussed this with Richard Smith, who points out that GCC does not accept this and it's not permitted according to the C++ standard, so this should probably be considered an accidental fix of a longstanding bug. With that said, would it be useful to add this as a tested case?

I think that would be a useful test case if only to document that we explicitly expect the situation to be a parsing error.

clang/lib/Parse/ParseStmt.cpp

if (Tok.is(tok::r_brace)) {
       return StmtError();
     }
 
     return ParseExprStatement(StmtCtx);
   }
 
   case tok::kw___attribute: {
     GNUAttributeLoc = Tok.getLocation();
+    Attrs.Range.setBegin(GNUAttributeLoc);
     ParseGNUAttributes(Attrs);
     goto Retry;
   }
 
   case tok::kw_case:                // C99 6.8.1: labeled-statement
     return ParseCaseStatement(StmtCtx);
   case tok::kw_default:             // C99 6.8.1: labeled-statement
     return ParseDefaultStatement(StmtCtx);