LGTM aside from a nit. You should give other reviewers a chance to sign off in case they have additional comments.

In D64838#1592520, @Nathan-Huckleberry wrote:

void foo() {
  __attribute__((address_space(0))) *x;
  *y;
}

If the attributes are parsed then function body looks like this to the parser:

{
  *x; //this one has attributes now
  *y;
{

The first line should be a valid declaration and the second like should be a dereference of an uninitialized variable. If the attributes token is discarded before parsing the rest of the line the only way to differentiate these is by looking at the attributes added to them.

An alternative may be parse the attributes list and immediately try to parse as a declaration then if that parsing fails attempt to parse as something else. Although this approach also has the scary implication of things that are supposed to be declarations getting reparsed as something entirely different.

LGTM!

In D64838#1592118, @Nathan-Huckleberry wrote:

In D64838#1592111, @aaron.ballman wrote:

In D64838#1589770, @Nathan-Huckleberry wrote:

The main problem that we have is that the __attribute__ token always causes the parser to read the line as a declaration. Then the declaration parser handles reading the attributes list.

This case demonstrates the problem:

void foo() {
  __attribute__((address_space(0))) *x;
}

If the attribute token is read beforehand this code just becomes *x and is parsed as a dereference of an undefined variable when it should actually be a declaration.

Maybe the best solution is to pull the attributes parsing out to ParseStatementOrDeclaration then pass those attributes through to following functions.
When the determination is made whether a line is a declaration or a statement the attributes list can be looked at to determine if the attributes are statement or declaration attributes and continue accordingly. Maybe throwing a warning if mixing of declaration and statement attributes occur.

Does that sound like a good solution?

Please see the discussion in https://reviews.llvm.org/D63299#inline-564887 -- if I understand your approach properly, this approach leads to spooky action at a distance, where the name of the attribute dictates whether something is a statement or a declaration. I'm not comfortable with that. There's no reason we could not have an attribute that is both a statement attribute and a declaration attribute, and how would *that* parse?

I think this requires some deeper surgery in the parsing logic so that attributes do not impact whether something is treated as a declaration or a statement. The parser should parse attributes first, keep them around for a while, and attach them to either the decl or the statement at a later point.

Any idea how to fix the problem in the code sample I gave? The addition of the attributes token causes code to be read as a declaration and without the token it is read as a unary operator.

In D64838#1589770, @Nathan-Huckleberry wrote:

The main problem that we have is that the __attribute__ token always causes the parser to read the line as a declaration. Then the declaration parser handles reading the attributes list.

This case demonstrates the problem:

void foo() {
  __attribute__((address_space(0))) *x;
}

If the attribute token is read beforehand this code just becomes *x and is parsed as a dereference of an undefined variable when it should actually be a declaration.

Maybe the best solution is to pull the attributes parsing out to ParseStatementOrDeclaration then pass those attributes through to following functions.
When the determination is made whether a line is a declaration or a statement the attributes list can be looked at to determine if the attributes are statement or declaration attributes and continue accordingly. Maybe throwing a warning if mixing of declaration and statement attributes occur.

Does that sound like a good solution?

There seems to be some separable concerns in this patch. I'd rather real with warn_unused_result, const, and pure attributes separately only because those are under the gnu attribute namespace and I'd prefer to match GNU's semantics for those attributes (or at least understand better where the inconsistencies are).

LGTM aside from a tiny nit.

I think this looks reasonable to me, though I am still not certain if the relative path in the python script will work with both the svn in-tree directory layout as well as the git monorepo layout (which I'm far less familiar with).

LGTM! Do you need someone to commit on your behalf?

In D63139#1579231, @rjmccall wrote:

I agree that tools shouldn't be forced to deal with invalid AST that looks like valid AST. To me that means finding ways to preserve information that (1) don't badly violate invariants and (2) are easily discoverable as invalid.

In D63192#1578867, @xbolva00 wrote:

I see. I should really check clang-tidy codebase :)

I'm wary of adding this as an on-by-default warning, despite its presence in MSVC (and despite submitting a similar patch years ago to implement the same functionality). Machine-generated code runs into this one frequently (we've been bitten by it numerous times with our tablegen code, especially in the backends), but it does not seem likely to occur in hand-written code very often.

In D64454#1578704, @Szelethus wrote:

Just thinking aloud!

We were tinkering with the idea of a checker creator script similar to what clang-tidy has, that could help on this potentially.

Is generating the rst code with Tblgen a feasable approach? At first glance, it sounds like a nightmare to implement, and I wonder whether the maintainers of sphinx buildbots would need to adjust to that.

In D63139#1561891, @rjmccall wrote:

Are the CaseStmts being dropped in C++ because the expression overflows? I agree that that's bad AST behavior; we should strive to generate AST whenever we can, even if it's not valid.

I'm worried that this will continue to drift out of sync with the static analyzer checks unless we find some way to automate it. I wonder if we could write a script to generate these .rst files and somehow fit it into the workflow for adding new static analyzer checks to re-run the script to produce new files (or modified files) as needed? @NoQ and @Szelethus may have ideas.

I've commit on your behalf in r365498. Thank you for the patch!

LGTM, but you should wait for others to have a chance to discuss before committing. You can always do the Clang docs in a separate follow-up patch (I know the changes are trivial, I just wanted to make sure they were all taken care of).

I think this patch should also update our user-facing documentation at the same time, such as https://clang.llvm.org/get_started.html, https://llvm.org/docs/GettingStarted.html, and https://llvm.org/docs/GettingStartedVS.html.

In D63845#1571855, @jdoerfert wrote:

I think we should postpone a detailed discussion until we are ready to send an RFC on this. Nevertheless, I inlined some responses below.

In D63845#1570639, @aaron.ballman wrote:

In D63845#1566987, @jdoerfert wrote:

In D63845#1561983, @lebedev.ri wrote:

In D63845#1561793, @jdoerfert wrote:

In D63845#1560605, @aaron.ballman wrote:

In D63845#1559995, @lebedev.ri wrote:

What's the target use-case here? What can't be solved with normal attributes?

With "normal" you mean something like __attribute__((noescape))? We don't have them for all attributes, I doubt we want to but I might be wrong.

That is precisely the question.
What is the motivation for exposing such LLVM-specific low-level unstable implementation detail?

I would disagree to the unstable part. Keeping LLVM-IR backward compatibility is always a priority and changing the meaning of an existing LLVM-IR attribute will break various things already.
Why would you think it is unstable?

Because, to date, we've never made blanket any stability guarantees about LLVM attributes from within the context of Clang. Instead, LLVM attributes are introduced into Clang on a case by case basis when they've been determined to be stable enough to warrant exposing. This extra layer also gives us more flexibility in translating frontend attributes into backend attributes.

You still assume LLVM attributes are not stable while I tried to argue they are, they have to be already for backward compatibility, at least target agnostic ones.

In D55793#1535895, @m4tx wrote:

@JonasToth thanks for asking! Yes, since I do not have commit access, I need someone to do the commit for me.

In D57086#1551354, @domdom wrote:

In D57086#1550514, @aaron.ballman wrote:

In D57086#1549632, @domdom wrote:

clang-format the patch

Thanks! Do you need someone to commit on your behalf?

You are very welcome; thank you both for your comments!

I do need someone to commit on my behalf :)

In D63845#1566987, @jdoerfert wrote:

In D63845#1561983, @lebedev.ri wrote:

In D63845#1561793, @jdoerfert wrote:

In D63845#1560605, @aaron.ballman wrote:

In D63845#1559995, @lebedev.ri wrote:

What's the target use-case here? What can't be solved with normal attributes?

With "normal" you mean something like __attribute__((noescape))? We don't have them for all attributes, I doubt we want to but I might be wrong.

That is precisely the question.
What is the motivation for exposing such LLVM-specific low-level unstable implementation detail?

I would disagree to the unstable part. Keeping LLVM-IR backward compatibility is always a priority and changing the meaning of an existing LLVM-IR attribute will break various things already.
Why would you think it is unstable?

In D63954#1562928, @erik.pilkington wrote:

We explicitly allow to add an annotation after
the definition of the class to allow adding annotations
to external source from by the user, e.g.

#include <vector>

namespace std {
template<typename T, typename Alloc>
class [[gsl::Owner(T)]] vector;
}

Wait, does that even work? What if the vector was declared in an inline namespace in the header? Seems like a strange recommendation for users. Is there some reason you can't just add these annotations to standard libraries? I doubt libcxx would have a problem with getting better warnings on their types.

In D63845#1562006, @wsmoses wrote:

Just to preface these comments -- this is work in progress and not intended to go in as is / without discussion (but happily is indeed inviting discussion).

In D63082#1560667, @xbolva00 wrote:

I wanted to follow GCC’s behaviour -Wall but then dicussion moved to “tautological compare group” - I was fine with that too..
I can again revert it to -Wall...
If this should be off by default even on -Wall, then all work was useless..

In terms of the code, I think this is ready to go. However, I'm still not happy with '*' in boolean context as a diagnostic. Perhaps appending something about the resulting value always being true|false would help most of these diagnostics be more obvious. blah in boolean context will always evaluate to true|false optionally followed by ; did you mean yada? would make it more obvious what is wrong with the code.

In D63845#1559995, @lebedev.ri wrote:

What's the target use-case here? What can't be solved with normal attributes?
I wonder if this should go to cfe+llvm -dev lists first, it's kinda intrusive.
I also wonder if all these should cause a clang diagnostic, at least under -Wall.
How is versioning expected to be handled? New attribute vs old clang, and vice versa.

LGTM with a few nits.

In D63423#1550768, @xbolva00 wrote:

Perhaps the author can run the check over a large corpus of code to see whether fps come up in practice? (The presence of fps will suggest to not warn in macros, but the absence of fps won't tell us too much.)

Sorry, I have no time nor spare computers to check big codebases. As @jfb said, let's start with a conservative approach and possibly improve it in the future.

In D63423#1550713, @jfb wrote:

In D63423#1550705, @aaron.ballman wrote:

In D63423#1550697, @jfb wrote:

What I meant with macros was that I don't think we should warn on:

#define LEGIT(a, b) ({ work work work; a ^ b; work work work; })

LEGIT(10, 5);

If the constants are inline in the macros then sure, warn there. Basically: if you literally wrote CONSTANT ^ CONSTANT and it looks fishy, let's warn. If token-pasting wrote it for you and it looks fishy, let's not warn.

So you would not want to see this code diagnosed?

#define POW(x, y) (x ^ y)

void f() {
  int two_to_the_derp = POW(2, 16);
}

I'm not certain I understand the rationale for why we would not want to diagnose such a construct. Do we have reason to expect a lot of false positives?

If you wrote CONSTANT_2_OR_10 ^ CONSTANT I'm pretty sure you messed up. Macros are opaque, and I'm not sure you clearly messed up anymore. Let's take the clear win, see what falls out, and only expand our reach if we have reason to do so. Without trying on huge codebases I don't know whether we'll get a lot of false positive. I'd rather be conservative and not cause headache.

In D63423#1550697, @jfb wrote:

What I meant with macros was that I don't think we should warn on:

#define LEGIT(a, b) ({ work work work; a ^ b; work work work; })

LEGIT(10, 5);

If the constants are inline in the macros then sure, warn there. Basically: if you literally wrote CONSTANT ^ CONSTANT and it looks fishy, let's warn. If token-pasting wrote it for you and it looks fishy, let's not warn.

In D63423#1550586, @xbolva00 wrote:

... But I go thru codesearch and this is just one case I think.

I think we should warn in macros... In codesearch case ,they are many more true positives, then false negatives.

#define MAX_T_U32 ((2^32)-1)

Ouch.

In D63423#1550563, @xbolva00 wrote:

We will be noisy in this case

#define IRQ_CHINT4_LEVEL        (11 ^ 7)
#define IRQ_CHINT3_LEVEL        (10 ^ 7)
#define IRQ_CHINT2_LEVEL        (9 ^ 7)
#define IRQ_CHINT1_LEVEL        (8 ^ 7)

In D63423#1547244, @regehr wrote:

In D63423#1547216, @xbolva00 wrote:

The only remaining question is, as your said, whether or not to diagnose xors in macros. @regerh @jfb ?

IMO it's better to not warn about xors in macros-- I like the current tradeoffs.

In D57086#1549632, @domdom wrote:

clang-format the patch

LGTM aside from a documentation nit.

The attribute parts LGTM! You can change the TargetItaniumCXXABI part in a follow-up commit if you'd prefer.

Committed in r363820; we'll look into integrating the script into lit as you suggested for a possible follow-up.

LGTM!

In D61552#1550080, @Tyker wrote:

fixed requested changes.

Are you getting errors from running it, or just incorrect output?

the issue happens to me even on master so i suppose the input is correct.
here is the error report:

[tyker@tyker tools]$ ./dump_ast_matchers.py 
*** Unparsable: " #ifndef LLVM_CLANG_ASTMATCHERS_ASTMATCHERS_H #define LLVM_CLANG_ASTMATCHERS_ASTMATCHERS_H " ***
*** Unparsable: " #include "clang/AST/ASTContext.h" #include "clang/AST/ASTTypeTraits.h" #include "clang/AST/Attr.h" #include "clang/AST/Decl.h" #include "clang/AST/DeclCXX.h" #include "clang/AST/DeclFriend.h" #include "clang/AST/DeclObjC.h" #include "clang/AST/DeclTemplate.h" #include "clang/AST/Expr.h" #include "clang/AST/ExprCXX.h" #include "clang/AST/ExprObjC.h" #include "clang/AST/NestedNameSpecifier.h" #include "clang/AST/OpenMPClause.h" #include "clang/AST/OperationKinds.h" #include "clang/AST/Stmt.h" #include "clang/AST/StmtCXX.h" #include "clang/AST/StmtObjC.h" #include "clang/AST/StmtOpenMP.h" #include "clang/AST/TemplateBase.h" #include "clang/AST/TemplateName.h" #include "clang/AST/Type.h" #include "clang/AST/TypeLoc.h" #include "clang/ASTMatchers/ASTMatchersInternal.h" #include "clang/ASTMatchers/ASTMatchersMacros.h" #include "clang/Basic/AttrKinds.h" #include "clang/Basic/ExceptionSpecificationType.h" #include "clang/Basic/IdentifierTable.h" #include "clang/Basic/LLVM.h" #include "clang/Basic/SourceManager.h" #include "clang/Basic/Specifiers.h" #include "clang/Basic/TypeTraits.h" #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Casting.h" #include "llvm/Support/Compiler.h" #include "llvm/Support/ErrorHandling.h" #include "llvm/Support/Regex.h" #include <cassert> #include <cstddef> #include <iterator> #include <limits> #include <string> #include <utility> #include <vector> " ***
*** Unparsable: " namespace clang {" ***
*** Unparsable: " using DeclarationMatcher = internal::Matcher<Decl>;" ***
*** Unparsable: " using StatementMatcher = internal::Matcher<Stmt>;" ***
*** Unparsable: " using TypeMatcher = internal::Matcher<QualType>;" ***
*** Unparsable: " using TypeLocMatcher = internal::Matcher<TypeLoc>;" ***
*** Unparsable: " using NestedNameSpecifierMatcher = internal::Matcher<NestedNameSpecifier>;" ***
*** Unparsable: " using NestedNameSpecifierLocMatcher = internal::Matcher<NestedNameSpecifierLoc>;" ***
*** Unparsable: " using CXXCtorInitializerMatcher = internal::Matcher<CXXCtorInitializer>;" ***
Probing https://clang.llvm.org/doxygen/classclang_1_1Decl.html...

after this the script keep probing with no issues.
i tested using python 2.7 and python 3.7 the same error occurs