Page MenuHomePhabricator

[clang] Set begin loc on GNU attribute parsed attrs
ClosedPublic

Authored by tbaeder on Mar 9 2020, 5:29 AM.

Details

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));
^

Diff Detail

Event Timeline

tbaeder created this revision.Mar 9 2020, 5:29 AM

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 ↗(On Diff #249069)

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 this revision to Diff 249574.Mar 11 2020, 3:33 AM

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 added a subscriber: rsmith.

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

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

tbaeder marked an inline comment as done.Mar 23 2020, 6:16 AM

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

clang/test/SemaCXX/switch-implicit-fallthrough.cpp
110–114

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 added inline comments.Mar 23 2020, 1:09 PM
clang/test/SemaCXX/switch-implicit-fallthrough.cpp
110–114

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.

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?

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.

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'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 updated this revision to Diff 334604.Thu, Apr 1, 12:15 AM

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.

tbaeder updated this revision to Diff 334605.Thu, Apr 1, 12:19 AM

Thank you for picking this back up!

clang/include/clang/Parse/Parser.h
2706–2707

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.

2706–2716

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

2723–2724

A similar comment here would be handy.

2723–2750

Same

2725–2726
2732–2733
clang/lib/Parse/ParseDecl.cpp
164–167

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

This test looks to be failing the CI pipeline currently.

tbaeder updated this revision to Diff 334668.Thu, Apr 1, 6:18 AM
tbaeder marked 10 inline comments as done.
This revision is now accepted and ready to land.Thu, Apr 1, 7:01 AM
This revision was landed with ongoing or failed builds.Thu, Apr 1, 8:27 AM
This revision was automatically updated to reflect the committed changes.

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. :(

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.

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

tbaeder updated this revision to Diff 334954.Fri, Apr 2, 7:38 AM

A little earlier than expected. Happy Easter :P

aaron.ballman added inline comments.Fri, Apr 2, 10:49 AM
clang/test/Parser/cxx0x-attributes.cpp
377

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 updated this revision to Diff 335016.Fri, Apr 2, 12:49 PM

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

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

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.

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.

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?

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.