[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 ↗

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

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

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

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

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.

[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/include/clang/Parse/Parser.h

private:
   }
   bool ConsumeAndStoreUntil(tok::TokenKind T1, tok::TokenKind T2,
                             CachedTokens &Toks,
                             bool StopAtSemi = true,
                             bool ConsumeFinalToken = true);
 
   //===--------------------------------------------------------------------===//
   // C99 6.9: External Definitions.
-  struct ParsedAttributesWithRange : ParsedAttributes {
-    ParsedAttributesWithRange(AttributeFactory &factory)
-      : ParsedAttributes(factory) {}
-
-    void clear() {
-      ParsedAttributes::clear();
-      Range = SourceRange();
-    }
-
-    SourceRange Range;
-  };
-  struct ParsedAttributesViewWithRange : ParsedAttributesView {
-    ParsedAttributesViewWithRange() : ParsedAttributesView() {}
-    void clearListOnly() {
-      ParsedAttributesView::clearListOnly();
-      Range = SourceRange();
-    }
-
-    SourceRange Range;
-  };
-
   DeclGroupPtrTy ParseExternalDeclaration(ParsedAttributesWithRange &attrs,
                                           ParsingDeclSpec *DS = nullptr);
   bool isDeclarationAfterDeclarator();
   bool isStartOfFunctionDefinition(const ParsingDeclarator &Declarator);
   DeclGroupPtrTy ParseDeclarationOrFunctionDefinition(
                                                   ParsedAttributesWithRange &attrs,
                                                   ParsingDeclSpec *DS = nullptr,
                                                   AccessSpecifier AS = AS_none);
private:
   void MaybeParseGNUAttributes(Declarator &D,
                                LateParsedAttrList *LateAttrs = nullptr) {
     if (Tok.is(tok::kw___attribute)) {
       ParsedAttributes attrs(AttrFactory);
       SourceLocation endLoc;
       ParseGNUAttributes(attrs, &endLoc, LateAttrs, &D);
       D.takeAttributes(attrs, endLoc);
     }
   }
-  bool MaybeParseGNUAttributes(ParsedAttributes &attrs,
-                               SourceLocation *endLoc = nullptr,
+
+  /// Parses GNU-style attributes and returns them without source range
+  /// information.
+  ///
+  /// This API is discouraged. Use the version that takes a
+  /// ParsedAttributesWithRange instead.
+  bool MaybeParseGNUAttributes(ParsedAttributes &Attrs,
+                               SourceLocation *EndLoc = nullptr,
                                LateParsedAttrList *LateAttrs = nullptr) {
     if (Tok.is(tok::kw___attribute)) {
-      ParseGNUAttributes(attrs, endLoc, LateAttrs);
+      ParsedAttributesWithRange AttrsWithRange(AttrFactory);
+      ParseGNUAttributes(Attrs, EndLoc, LateAttrs);
+      Attrs.takeAllFrom(AttrsWithRange);
       return true;
     }
     return false;
   }
-  void ParseGNUAttributes(ParsedAttributes &attrs,
-                          SourceLocation *endLoc = nullptr,
+
+  bool MaybeParseGNUAttributes(ParsedAttributesWithRange &Attrs,
+                               SourceLocation *EndLoc = nullptr,
+                               LateParsedAttrList *LateAttrs = nullptr) {
+    if (Tok.is(tok::kw___attribute)) {
+      ParseGNUAttributes(Attrs, EndLoc, LateAttrs);
+      return true;
+    }
+    return false;
+  }
+
+  /// Parses GNU-style attributes and returns them without source range
+  /// information.
+  ///
+  /// This API is discouraged. Use the version that takes a
+  /// ParsedAttributesWithRange instead.
+  void ParseGNUAttributes(ParsedAttributes &Attrs,
+                          SourceLocation *EndLoc = nullptr,
+                          LateParsedAttrList *LateAttrs = nullptr,
+                          Declarator *D = nullptr) {
+    ParsedAttributesWithRange AttrsWithRange(AttrFactory);
+    ParseGNUAttributes(AttrsWithRange, EndLoc, LateAttrs, D);
+    Attrs.takeAllFrom(AttrsWithRange);
+  }
+
+  void ParseGNUAttributes(ParsedAttributesWithRange &Attrs,
+                          SourceLocation *EndLoc = nullptr,
                           LateParsedAttrList *LateAttrs = nullptr,
                           Declarator *D = nullptr);
   void ParseGNUAttributeArgs(IdentifierInfo *AttrName,
                              SourceLocation AttrNameLoc,
                              ParsedAttributes &Attrs, SourceLocation *EndLoc,
                              IdentifierInfo *ScopeName, SourceLocation ScopeLoc,
                              ParsedAttr::Syntax Syntax, Declarator *D);
   IdentifierLoc *ParseIdentifierLoc();

clang/include/clang/Sema/ParsedAttr.h

addNewPropertyAttr(IdentifierInfo *attrName, SourceRange attrRange,
     addAtEnd(attr);
     return attr;
   }
 
 private:
   mutable AttributePool pool;
 };
 
+struct ParsedAttributesWithRange : ParsedAttributes {
+  ParsedAttributesWithRange(AttributeFactory &factory)
+      : ParsedAttributes(factory) {}
+
+  void clear() {
+    ParsedAttributes::clear();
+    Range = SourceRange();
+  }
+
+  SourceRange Range;
+};
+struct ParsedAttributesViewWithRange : ParsedAttributesView {
+  ParsedAttributesViewWithRange() : ParsedAttributesView() {}
+  void clearListOnly() {
+    ParsedAttributesView::clearListOnly();
+    Range = SourceRange();
+  }
+
+  SourceRange Range;
+};
+
 /// These constants match the enumerated choices of
 /// err_attribute_argument_n_type and err_attribute_argument_type.
 enum AttributeArgumentNType {
   AANT_ArgumentIntOrBool,
   AANT_ArgumentIntegerConstant,
   AANT_ArgumentString,
   AANT_ArgumentIdentifier,
   AANT_ArgumentConstantExpr,

clang/lib/Parse/ParseDecl.cpp

 ///    wanted an expression, it's out of luck (but it turns out that no
 ///    attributes work that way, because C constant expressions are very
 ///    limited).
 ///  * In C++ mode, if the attribute argument list starts with an identifier,
 ///    and the attribute *wants* an identifier, it is parsed as an identifier.
 ///    At block scope, any additional tokens between the identifier and the
 ///    ',' or ')' are ignored, otherwise they produce a parse error.
 ///
 /// We follow the C++ model, but don't allow junk after the identifier.
-void Parser::ParseGNUAttributes(ParsedAttributes &attrs,
-                                SourceLocation *endLoc,
-                                LateParsedAttrList *LateAttrs,
-                                Declarator *D) {
+void Parser::ParseGNUAttributes(ParsedAttributesWithRange &Attrs,
+                                SourceLocation *EndLoc,
+                                LateParsedAttrList *LateAttrs, Declarator *D) {
   assert(Tok.is(tok::kw___attribute) && "Not a GNU attribute list!");
 
+  SourceLocation StartLoc = Tok.getLocation(), Loc;
+
+  if (!EndLoc)
+    EndLoc = &Loc;
+
   while (Tok.is(tok::kw___attribute)) {
     SourceLocation AttrTokLoc = ConsumeToken();
-    unsigned OldNumAttrs = attrs.size();
+    unsigned OldNumAttrs = Attrs.size();
     unsigned OldNumLateAttrs = LateAttrs ? LateAttrs->size() : 0;
 
     if (ExpectAndConsume(tok::l_paren, diag::err_expected_lparen_after,
                          "attribute")) {
       SkipUntil(tok::r_paren, StopAtSemi); // skip until ) or ;
       return;
     }
     if (ExpectAndConsume(tok::l_paren, diag::err_expected_lparen_after, "(")) {
do {
         break;
       IdentifierInfo *AttrName = Tok.getIdentifierInfo();
       if (!AttrName)
         break;
 
       SourceLocation AttrNameLoc = ConsumeToken();
 
       if (Tok.isNot(tok::l_paren)) {
-        attrs.addNew(AttrName, AttrNameLoc, nullptr, AttrNameLoc, nullptr, 0,
+        Attrs.addNew(AttrName, AttrNameLoc, nullptr, AttrNameLoc, nullptr, 0,
                      ParsedAttr::AS_GNU);
         continue;
       }
 
       // Handle "parameterized" attributes
       if (!LateAttrs || !isAttributeLateParsed(*AttrName)) {
-        ParseGNUAttributeArgs(AttrName, AttrNameLoc, attrs, endLoc, nullptr,
+        ParseGNUAttributeArgs(AttrName, AttrNameLoc, Attrs, EndLoc, nullptr,
                               SourceLocation(), ParsedAttr::AS_GNU, D);
         continue;
       }
 
       // Handle attributes with arguments that require late parsing.
       LateParsedAttribute *LA =
           new LateParsedAttribute(this, *AttrName, AttrNameLoc);
       LateAttrs->push_back(LA);
do {
       LA->Toks.push_back(Eof);
     } while (Tok.is(tok::comma));
 
     if (ExpectAndConsume(tok::r_paren))
       SkipUntil(tok::r_paren, StopAtSemi);
     SourceLocation Loc = Tok.getLocation();
     if (ExpectAndConsume(tok::r_paren))
       SkipUntil(tok::r_paren, StopAtSemi);
-    if (endLoc)
-      *endLoc = Loc;
+    if (EndLoc)
+      *EndLoc = Loc;
 
     // If this was declared in a macro, attach the macro IdentifierInfo to the
     // parsed attribute.
     auto &SM = PP.getSourceManager();
     if (!SM.isWrittenInBuiltinFile(SM.getSpellingLoc(AttrTokLoc)) &&
         FindLocsWithCommonFileID(PP, AttrTokLoc, Loc)) {
       CharSourceRange ExpansionRange = SM.getExpansionRange(AttrTokLoc);
       StringRef FoundName =
           Lexer::getSourceText(ExpansionRange, SM, PP.getLangOpts());
       IdentifierInfo *MacroII = PP.getIdentifierInfo(FoundName);
 
-      for (unsigned i = OldNumAttrs; i < attrs.size(); ++i)
-        attrs[i].setMacroIdentifier(MacroII, ExpansionRange.getBegin());
+      for (unsigned i = OldNumAttrs; i < Attrs.size(); ++i)
+        Attrs[i].setMacroIdentifier(MacroII, ExpansionRange.getBegin());
 
       if (LateAttrs) {
         for (unsigned i = OldNumLateAttrs; i < LateAttrs->size(); ++i)
           (*LateAttrs)[i]->MacroII = MacroII;
       }
     }
   }
+
+  Attrs.Range = SourceRange(StartLoc, *EndLoc);
 }
 
 /// Determine whether the given attribute has an identifier argument.
 static bool attributeHasIdentifierArg(const IdentifierInfo &II) {
 #define CLANG_ATTR_IDENTIFIER_ARG_LIST
   return llvm::StringSwitch<bool>(normalizeAttrName(II.getName()))
 #include "clang/Parse/AttrParserStringSwitches.inc"
            .Default(false);

clang/test/AST/sourceranges.cpp

void f() {
     int __attribute__((unused)) i3;
     // CHECK: DeclStmt {{.*}} <<built-in>:{{.*}}, {{.*}}:[[@LINE+1]]:40>
     __declspec(dllexport) extern int i4;
     // CHECK: DeclStmt {{.*}} <line:[[@LINE+1]]:5, col:40>
     extern int __declspec(dllexport) i5;
   }
 }
 
+// CHECK-1Z: NamespaceDecl {{.*}} attributed_case
+namespace attributed_case {
+void f(int n) {
+  switch (n) {
+  case 0:
+    n--;
+    // CHECK: AttributedStmt {{.*}} <line:[[@LINE+2]]:5, line:[[@LINE+4]]:35>
+    // CHECK: FallThroughAttr {{.*}} <line:[[@LINE+1]]:20>
+    __attribute__((fallthrough))
+    // CHECK: FallThroughAttr {{.*}} <line:[[@LINE+1]]:22>
+      __attribute__((fallthrough));
+  case 1:
+    n++;
+    break;
+  }
+}
+} // namespace attributed_case
+
 // CHECK: NamespaceDecl {{.*}} attributed_stmt
 namespace attributed_stmt {
   // In DO_PRAGMA and _Pragma cases, `LoopHintAttr` comes from <scratch space>
   // file.
 
   #define DO_PRAGMA(x) _Pragma (#x)
 
   void f() {

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

switch (n / 50) {
     case 21: { // no warning here, there's no fall-through
       break;
     }
     case 23: // no warning here, there's no fall-through
       n += 128000;
       break;
     case 25: // no warning here, there's no fall-through
       break;
   }
 
   return n;
 }
 
 class ClassWithDtor {
 public:
   ~ClassWithDtor() {}
 };
 
 void fallthrough2(int n) {
   switch (n) {
     case 0:

 int fallthrough_position(int n) {
   switch (n) {
       n += 300;
       [[clang::fallthrough]];  // expected-warning{{fallthrough annotation in unreachable code}}
     case 221:
       return 1;
       [[clang::fallthrough]];  // expected-warning{{fallthrough annotation in unreachable code}}
     case 222:
+      return 2;
+      __attribute__((fallthrough)); // expected-warning{{fallthrough annotation in unreachable code}}
+    case 223:
       n += 400;
-    case 223:          // expected-warning{{unannotated fall-through between switch labels}} expected-note{{insert '[[clang::fallthrough]];' to silence this warning}} expected-note{{insert 'break;' to avoid fall-through}}
+    case 224: // expected-warning{{unannotated fall-through between switch labels}} expected-note{{insert '[[clang::fallthrough]];' to silence this warning}} expected-note{{insert 'break;' to avoid fall-through}}
         ;
   }
 
   long p = static_cast<long>(n) * n;
   switch (sizeof(p)) {
     case 9:
       n += static_cast<int>(p >> 32);
       [[clang::fallthrough]];  // no warning here
     case 5: