Fix ast print of variables with attributes

[giulianobelinassi]

Summary

Previously clang AST prints the following declaration:

int fun_var_unused() {

int x __attribute__((unused)) = 0;
return x;

}

and

int __declspec(thread) x = 0;

as:

int fun_var_unused() {

int x = 0 __attribute__((unused));
return x;

}

and

int x = __declspec(thread) 0;

which is rejected by C/C++ parser. This patch modifies the logic to
print old C attributes for variables as:

int __attribute__((unused)) x = 0;

and the __declspec case as:

int __declspec(thread) x = 0;

Fixes: https://github.com/llvm/llvm-project/issues/59973

Previous version: D141714.

Signed-off-by: Giuliano Belinassi <[e-mail]>

[aaron.ballman]

Thank you for the fix!

It looks like precommit CI found a related failure that needs to be addressed: https://buildkite.com/llvm-project/premerge-checks/builds/130589#0185ac99-1158-46b3-b6d1-52fcf5310a59

Can you also add a release note about the fix as well?

clang/lib/AST/DeclPrinter.cpp

969

Adding in a comment for the problems that remain.

[giulianobelinassi]

In D141714#4077150, @aaron.ballman wrote:

Thank you for the fix!

It looks like precommit CI found a related failure that needs to be addressed: https://buildkite.com/llvm-project/premerge-checks/builds/130589#0185ac99-1158-46b3-b6d1-52fcf5310a59

Can you also add a release note about the fix as well?

Thank you for your review!

I will update this patch once I get spare cycles to this. There is also an additonal case regarding __declspec and K&R functions that needs to be addressed as well in this patch which it currently doesn't do.

[aaron.ballman]

In D141714#4077199, @giulianobelinassi wrote:

In D141714#4077150, @aaron.ballman wrote:

Thank you for the fix!

It looks like precommit CI found a related failure that needs to be addressed: https://buildkite.com/llvm-project/premerge-checks/builds/130589#0185ac99-1158-46b3-b6d1-52fcf5310a59

Can you also add a release note about the fix as well?

Thank you for your review!

I will update this patch once I get spare cycles to this. There is also an additonal case regarding __declspec and K&R functions that needs to be addressed as well in this patch which it currently doesn't do.

You don't have to worry about the additional cases (unless you want to, but then they can be handled in separate patches); ast pretty printing is wrong, broken, and totally incorrect in a whole lot of places; we maintain it as a best effort as a debugging aid.

[giulianobelinassi]

In D141714#4077204, @aaron.ballman wrote:

In D141714#4077199, @giulianobelinassi wrote:

In D141714#4077150, @aaron.ballman wrote:

Thank you for the fix!

It looks like precommit CI found a related failure that needs to be addressed: https://buildkite.com/llvm-project/premerge-checks/builds/130589#0185ac99-1158-46b3-b6d1-52fcf5310a59

Can you also add a release note about the fix as well?

Thank you for your review!

I will update this patch once I get spare cycles to this. There is also an additonal case regarding __declspec and K&R functions that needs to be addressed as well in this patch which it currently doesn't do.

You don't have to worry about the additional cases (unless you want to, but then they can be handled in separate patches); ast pretty printing is wrong, broken, and totally incorrect in a whole lot of places; we maintain it as a best effort as a debugging aid.

That is interesting. I am developing a static analyzer which relies on this for outputing code, so I would need those issues to be fixed for that project to succeed. If you have additional cases already mapped and you want to share with me I will happily fix them as well.

[aaron.ballman]

In D141714#4077207, @giulianobelinassi wrote:

In D141714#4077204, @aaron.ballman wrote:

In D141714#4077199, @giulianobelinassi wrote:

In D141714#4077150, @aaron.ballman wrote:

Thank you for the fix!

It looks like precommit CI found a related failure that needs to be addressed: https://buildkite.com/llvm-project/premerge-checks/builds/130589#0185ac99-1158-46b3-b6d1-52fcf5310a59

Can you also add a release note about the fix as well?

Thank you for your review!

I will update this patch once I get spare cycles to this. There is also an additonal case regarding __declspec and K&R functions that needs to be addressed as well in this patch which it currently doesn't do.

You don't have to worry about the additional cases (unless you want to, but then they can be handled in separate patches); ast pretty printing is wrong, broken, and totally incorrect in a whole lot of places; we maintain it as a best effort as a debugging aid.

That is interesting. I am developing a static analyzer which relies on this for outputing code, so I would need those issues to be fixed for that project to succeed. If you have additional cases already mapped and you want to share with me I will happily fix them as well.

The way Clang handles this is to mostly go back to the original source code (through the source manager and source location information) to grab what the user actually wrote. The pretty printing functionality loses information like whether something was expanded from a macro, etc and so, if the goal is to show what the user wrote, it's likely to be a really big uphill battle to get that through the pretty printer. (The other issue is that we don't actively maintain the pretty printer, so it's quite frequently the case that newer language constructs are not well-supported by the pretty printer unless it just so happens to work by default.)

[giulianobelinassi]

The way Clang handles this is to mostly go back to the original source code (through the source manager and source location information) to grab what the user actually wrote. The pretty printing functionality loses information like whether something was expanded from a macro, etc and so, if the goal is to show what the user wrote, it's likely to be a really big uphill battle to get that through the pretty printer.

This is something I already do, but there are some cases where I simply can not track down the origin of the declaration (some very complex macro expansions) and in this case I simply fall back to the prettyprinter. Those are the cases where I am interesting in fixing the prettyprinter. It is also worth noting that I am currently interested in C code so perhaps most of these issues don't manifest in this case.

[aaron.ballman]

In D141714#4077631, @giulianobelinassi wrote:

The way Clang handles this is to mostly go back to the original source code (through the source manager and source location information) to grab what the user actually wrote. The pretty printing functionality loses information like whether something was expanded from a macro, etc and so, if the goal is to show what the user wrote, it's likely to be a really big uphill battle to get that through the pretty printer.

This is something I already do, but there are some cases where I simply can not track down the origin of the declaration (some very complex macro expansions) and in this case I simply fall back to the prettyprinter. Those are the cases where I am interesting in fixing the prettyprinter. It is also worth noting that I am currently interested in C code so perhaps most of these issues don't manifest in this case.

Ah, good to know; that should hopefully make it easier for you.

[giulianobelinassi]

Update to dump attributes right after the type specification, also
fixing the __declspec case.

[aaron.ballman]

clang/lib/AST/DeclPrinter.cpp

52–58

60–61

252–254

303

FWIW, I'm more used to seeing __declspec and __attribute__ leading the declaration specifiers instead of trailing them. Also, "middle" raises questions for me as to where the attribute will go for a function. Given:

__attribute__((foo)) void bar(void);

what happens? All the test coverage that's changed currently is for variables, not for functions, so I can't really tell.

Also, this should be tested with keyword attributes like alignas and __global because those have very specific parse orders.

309–310

This comment isn't quite accurate -- it's more that [[]] attributes have very specific rules about what they appertain to based on the syntactic location of the [[]]. e.g.,

[[foo]] int a, b; // foo applies to both a and b
int [[foo]] a, b; // foo applies to the type of int, which forms the type for both a and b
int a [[foo]], b; // foo applies to just a

Also, the same logic applies to [[]] in C as well as C++, so I think you meant to use A->isStandardAttributeSyntax().

clang/test/Analysis/blocks.mm

73–74

Can you explain a bit more about what warning you're seeing and under what condition?

clang/test/Sema/attr-print.c

4

Why did you change where the attribute is written in this test?

8–11

Same questions here.

clang/test/SemaCXX/attr-print.cpp

3–4

Same questions here as above.

[giulianobelinassi]

Hi, Alan. Thanks for your review again!

With regard to middle, the patch sent in D145269 may answer your questions. Basically functions like:

int* f(void) __attribute__((unused));

would be output as

int* __attribute__((unused)) f(void);

if SIDE_MIDDLE is specified.

The case I want to avoid is:

int f(void) __attribute__((unused))
  int i;
{ return 0; }

Which is not clear where should the attribute be applied to. A left-to-right parser that accepts attributes on the right side of a function declaration may apply this to f, but if it don't then it will be applied to i. This is why GCC rejects this kind of attribute output on the right side of functions.
Hence, I thought it would be better to always output variable as

int __attribute__((unused)) var;

when possible. I also found that __declspec attributes is also accepted this way. But if that changes the meaning of things (I looked into generated assembly and could not find such case) then I think dumping it on the right side may be a better option, and we only have to be careful with functions declarations, those being dumped as.

__attribute__((unused)) f(void) {}

---

As for changing the __declspec locations, I thought it was fine as it is also accepted by clang. But looking at MSVC documentation it says that outputing it at the begining is recommended so I think it may be better to always output __declspecs on the left side?

---

Can you explain a bit more about what warning you're seeing and under what condition?

Basically if you change this test in order for it to intentionally fail because of output mismatch, this warning is generated.

[aaron.ballman]

In D141714#4175263, @giulianobelinassi wrote:

Hi, Alan. Thanks for your review again!

With regard to middle, the patch sent in D145269 may answer your questions. Basically functions like:

int* f(void) __attribute__((unused));

would be output as

int* __attribute__((unused)) f(void);

if SIDE_MIDDLE is specified.

That would be a pretty unique location for those attributes to go and there are some attributes that can't go there. For example: https://godbolt.org/z/fo5cnEGTY

The case I want to avoid is:

int f(void) __attribute__((unused))
  int i;
{ return 0; }

Did you mean the example to be a K&R C function instead? If so, then this would apply the attribute to the function in Clang.

Which is not clear where should the attribute be applied to. A left-to-right parser that accepts attributes on the right side of a function declaration may apply this to f, but if it don't then it will be applied to i. This is why GCC rejects this kind of attribute output on the right side of functions.
Hence, I thought it would be better to always output variable as

int __attribute__((unused)) var;

That will subtly change program semantics in Clang though, as now the attribute pretty prints to the parameter declaration (I'm still assuming your example was intended to be a K&R C function definition).

when possible. I also found that __declspec attributes is also accepted this way. But if that changes the meaning of things (I looked into generated assembly and could not find such case) then I think dumping it on the right side may be a better option, and we only have to be careful with functions declarations, those being dumped as.

__attribute__((unused)) f(void) {}

---

As for changing the __declspec locations, I thought it was fine as it is also accepted by clang. But looking at MSVC documentation it says that outputing it at the begining is recommended so I think it may be better to always output __declspecs on the left side?

That's where I'm most used to seeing it.

---

Can you explain a bit more about what warning you're seeing and under what condition?

Basically if you change this test in order for it to intentionally fail because of output mismatch, this warning is generated.

Hmmm, I'm still not certain I understand what's going on. What surprises me is that the test code is __block StructWithCopyConstructor s(5); but it pretty prints as StructWithCopyConstructor __attribute__((blocks("byref"))) s(5); which doesn't compile at all. That doesn't seem to be new behavior in your patch (and this particular attribute has no documentation or existing test coverage), but I think the fact that the keyword spelling is dropped and the attribute is moved helps point out an issue -- keyword attributes are special and their syntactic position is important, so maybe those should not shift at all.

[giulianobelinassi]

Hi, Aron.

Just to make myself clear: What I need to do is that the clang dumps for C files are also accepted by GCC as input.

Here is why I wanted to output the attribute on middle:

https://godbolt.org/z/6aPc6aWcz

As you can see, GCC complains of __attributes__ output on the right side of functions with bodies.

But overall, perhaps what should be output in the dumps are the following (please check if you agree with me):

1- Attributes in variables should be output to the right side, as it was done before. That is:

int var __attribute__((unused)) = 0;

2- Variables or functions declared with __declspec attributes should go to the left side, as does MSVC says is recommended. That is:

__declspec(thread) int var = 0;
__declspec(noinline) int f(void);
__declspec(noinline) int f(void) { return 0; }

3- Functions prototypes should have its attributes output to the right side, that means:

int f(void) __attribute__((unused));

4- But attributes specified in function declaration with body should go to the left side because GCC rejects outputing them on the right side, as:

__attribute__((unused)) int f(void) { return 0; }

---

The result of this choice would be that the following K&R function as input (notice how it is not clear where the attribute is being applied to:

int f(i)
 __attribute__((unused)) int i;
{ return 0; }

would be dumped as:

__attribute__((unused)) int f(i)
int i;
{ return 0; }

But in practical terms, GCC would accept it without problems and it is clear where the attribute is being applied to. Outputting to the right side has some ambiguity here, which is what I want to avoid.

[aaron.ballman]

In D141714#4175942, @giulianobelinassi wrote:

Hi, Aron.

Just to make myself clear: What I need to do is that the clang dumps for C files are also accepted by GCC as input.

FWIW, that's not a supported use for -ast-print. We've never had a requirement that -ast-print be useful for anything more than debugging scenarios and so the printing functionality *frequently* produces incorrect output, especially on newer language constructs. We fix up the functionality in an ad hoc manner as people find a reason to care about a particular situation.

If your goal is so that -ast-print reliably produces compilable code, I think that requires buy-in from the Clang community because that's a bit of a heavy lift for the community to support. Traditionally, we've pointed users to other tools that are usually better-suited to the task trying to be solved (like clang-format for pretty printing or stencils for performing source to source transformations, etc).

Here is why I wanted to output the attribute on middle:

https://godbolt.org/z/6aPc6aWcz

As you can see, GCC complains of __attributes__ output on the right side of functions with bodies.

But overall, perhaps what should be output in the dumps are the following (please check if you agree with me):

1- Attributes in variables should be output to the right side, as it was done before. That is:

int var __attribute__((unused)) = 0;

Agreed. FWIW, I don't mind if that winds up producing:

int var1 __attribute__((unused)) = 0, var2 __attribute__((unused)) = 0;

instead of:

__attribute__((unused)) int var1 = 0, var2 = 0;

2- Variables or functions declared with __declspec attributes should go to the left side, as does MSVC says is recommended. That is:

__declspec(thread) int var = 0;
__declspec(noinline) int f(void);
__declspec(noinline) int f(void) { return 0; }

Agreed.

3- Functions prototypes should have its attributes output to the right side, that means:

int f(void) __attribute__((unused));

So long as this doesn't change program meaning for any attributes, that seems reasonable enough.

4- But attributes specified in function declaration with body should go to the left side because GCC rejects outputing them on the right side, as:

__attribute__((unused)) int f(void) { return 0; }

This will break code for folks compiling with Clang, though, so it has to be done on a case-by-case basis. Any attribute accepting an expression argument that appears on the right of the function needs to continue to appear to the right of the function the expression refers to a parameter name. The same is true for use with lambdas.

---

The result of this choice would be that the following K&R function as input (notice how it is not clear where the attribute is being applied to:

int f(i)
 __attribute__((unused)) int i;
{ return 0; }

would be dumped as:

__attribute__((unused)) int f(i)
int i;
{ return 0; }

But in practical terms, GCC would accept it without problems and it is clear where the attribute is being applied to. Outputting to the right side has some ambiguity here, which is what I want to avoid.

Again, this only works for some attributes. Consider: https://godbolt.org/z/3YreGY1G4

There's another case to think about, which are tag types: https://godbolt.org/z/KjKn7rz89

And finally, there's [[]] attributes where changing the position from what's in source will potentially change what the attribute appertains to and also break code. So I think to achieve the goal you're after, there's actually quite a bit of work to be done. However, so long as the output doesn't get *worse* for other situations, I think any incremental progress here is acceptable. e.g., fixing this kind of nonsense is a good incremental step even if we do nothing else:

// MS-EXT-NEXT: int x = 3 __declspec(thread);
int __declspec(thread) x = 3;

[giulianobelinassi]

Update patch with the agreements of last discussion.

This new version includes code to handle the cases presented by Aron,
which are:

1- Variable with attributes have its attribute dumped before its value.

2- __declspec attributes are dumped on the left side of the declaration,
   as recommended by MSVC docs.

3- Functions with body has its attributes dumped on the right side of
   the declaration instead of left side when possible.  The point of
   this is to (1) avoid attribute placement confusion in K&R C
   functions and (2) keep compatibility with GCC. The case where
   it is possible to have a variable referenced in the attribute is
   also handled specially (AttributeIf).

[aaron.ballman]

clang/lib/AST/DeclPrinter.cpp

52–58

I think we should use an enum class just so we don't steal these identifiers at the global scope within this file, WDYT?

264–266

267–268

Minor style nits.

272

This should also handle C2x attributes, so I'd use isStandardAttributeSyntax() instead.

279

There are other attributes for which this is true as well, like enable_if, thread safety annotations, etc.

286–287

667–668

What's the purpose to hoisting these two lines?

clang/test/Analysis/blocks.mm

81

I can't quite tell if this change is good, bad, or just different.

[giulianobelinassi]

Hi. See inline answers. I will send the new version in a few minutes.

clang/lib/AST/DeclPrinter.cpp

52–58

Unfortunately that would result in necessary auxiliary code to do an bitwise '&' operation, so I don't think this is a good idea. Although I've explicitly now access those constants by using the AttrPrintLoc:: keyword rather than directly referencing the constant directly.

272

Done.

279

I have expanded this to enable_if, as it is trivial. The thread safety annotations is not that trivial and seems to not trigger any test failure. So I think I will leave this to a next patch that properly parses the attributes to find references to symbols declared in function argument.

clang/test/Analysis/blocks.mm

81

This indeed doesn't look good, but for what it is worth it is still corretly accepted by clang and gcc.

[giulianobelinassi]

Incorporate some of Aron suggestions:

• Replace isDeclspecAttribute with isStandardAttributeSyntax
• Avoid multiple calls to getAsFunction
• Use AttrPrintLoc:: instead of referencing the value directly
• Also output enable_if on the right side of decl.

[giulianobelinassi]

@aaron.ballman Ping

[aaron.ballman]

It looks like several comments are left unaddressed, are you planning on making the suggested changes?

clang/lib/AST/DeclPrinter.cpp

52–58

We have helper functionality for that, see LLVM_MARK_AS_BITMASK_ENUM from https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/ADT/BitmaskEnum.h

clang/test/Analysis/blocks.mm

81

I think this is a regression in terms of readability, but perhaps it's one we can live with.

[erichkeane]

clang/lib/AST/DeclPrinter.cpp

270

I think this is something we need to just do the right way, right away. The below is completely unsustainable, and is just going to encourage us to spend the next few years messing with this if/else-if tree. I'll leave final judgement to Aaron, but just making this its own function dependent on TableGen'ed files seems like what we should be doing from the start.

clang/test/Analysis/blocks.mm

81

So I have a BIG concern here. The primary purpose of AST print is to be readable. I don't think we should be willing to compromise that for what is, to the project, a non-goal.

[aaron.ballman]

clang/lib/AST/DeclPrinter.cpp

270

As much as I hate to obligate anyone to get involved in tablegen to solve a problem, I share the concern that this is not an extensible approach. I think @giulianobelinassi should move forward by trying to emit this from ClangAttrEmitter.cpp if at all possible.

clang/test/Analysis/blocks.mm

81

I think this issue would be resolved by going with a tablegen approach -- this attribute exists because of the __block keyword, so ideally, we wouldn't even be printing __attribute__ here.

[giulianobelinassi]

I will update the patch with the suggestions and send them again.

clang/lib/AST/DeclPrinter.cpp

270

I will do the TableGen approach, then..

[giulianobelinassi]

• Use tblgen to generate table of attributes that can or must be print on the left side of the Decl.
• Use LLVM_MARK_AS_BITMASK_ENUM on AttrPrintLoc enum.
• Print attributes of declarations contatining parenthesis ctors on the left side. Improves the case:
  
  StructWithCopyConstructor s attribute((blocks("byref")))(5)
  
  now printing:
  
  attribute((blocks("byref"))) StructWithCopyConstructor s(5)

[giulianobelinassi]

Hello,

It took me a while, but here it is the newer version of the patch with the tablegen stuff. Please reach to me if something needs to be changed in this regard.

This also improves the readability of declarations of variables that have a parenthesis constructor (like the __block case).

Thanks,
Giuliano.

[strimo378]

@giulianobelinassi I provided a similar patch as you addressed here, see https://reviews.llvm.org/D157394 . It looks like we have the same requirement that we both need compilable ast-print code. Are you interested in a teams meeting to discuss the topic and align our effort? If yes please write me an mail to [e-mail] .

[strimo378]

@aaron.ballman I had today a teams meeting with @giulianobelinassi and we discussed both patches as well as that we want to work together in improving AST print. Both patches are fine for us since they improve the attribute printing. The features of the respective other patch can be easily integrated with minimal changes in a subsequent smaller patch. We further think that there are a lot more cases of attributes that we need to consider that we do not know atm.

I think we should go on with the patch that has the higher probability to get landed soon. Since this branch had a lot more reviews, I suggest we go further with this patch. I further suggest to keep it simple and remove the tblgen part and add it in an subsequent patch once we know all cases and requirements. What do you think?

[giulianobelinassi]

Ping.

[erichkeane]

I think we can do better naming the tablegen'ed parts, else a bunch of smaller changes. Approach seems good enough to me, though Aaron should scroll through/make a determination after you've fixed my concerns.

clang/include/clang/Basic/Attr.td

565 ↗

(On Diff #547477)

566 ↗

(On Diff #547477)

567 ↗

(On Diff #547477)

568 ↗

(On Diff #547477)

clang/lib/AST/DeclPrinter.cpp

262

Don't include curley brackets here.

263

271

279

Same here, don't use curleys on single-liners.

279

968–980

Instead of making this a std::string, make it a StringRef and just assign it on line 1005, it looks like both sides of that return a StringRef (deuglifiedName and getName), and your += is unnecessary (since there is nothing in Name here anyway).

clang/utils/TableGen/TableGen.cpp

282 ↗

(On Diff #547477)

Unrelated changes, please remove.

[giulianobelinassi]

Apply @erichkeane suggestions

[erichkeane]

Looks fine to me, please don't commit for a day or two to give @aaron.ballman a chance to make a final comment.

[giulianobelinassi]

In D141714#4638225, @erichkeane wrote:

Looks fine to me, please don't commit for a day or two to give @aaron.ballman a chance to make a final comment.

I am sorry, but how am I supposed to commit those changes to main if I do not have write permission?

[erichkeane]

In D141714#4640825, @giulianobelinassi wrote:

In D141714#4638225, @erichkeane wrote:

Looks fine to me, please don't commit for a day or two to give @aaron.ballman a chance to make a final comment.

I am sorry, but how am I supposed to commit those changes to main if I do not have write permission?

I didn't realize you didn't have commit permissions! If you send me (either here, or privately) your name and email in the form of: "Full Name <[e-mail]>" I can do it for you

[erichkeane]

clang/lib/AST/DeclPrinter.cpp

279

While compiling this, I discovered that the lack of curley braces on the else if (canPrintOnLeftSide(A)) means that this else-if is actually an else to THAT instead, despite its indention (which implies it should be associated with the if (const FunctionDecl...).

Which is it? I presume the indenting is what you mean, but it hasn't been tested in a way that matters. Can you add a test that validates the var-decl printing on the LHS ONLY happens when it 'can' print on the LHS?

[giulianobelinassi]

clang/lib/AST/DeclPrinter.cpp

279

Sorry for the warning there. I will post a fixed version of this as soon as my build with -Werror=all completes.

Just to be clear, the intended code with braces is:

AttrPrintLoc AttrLoc = AttrPrintLoc::Right;
if (mustPrintOnLeftSide(A))
  // If we must always print on left side (e.g. declspec), then mark as
  // so.
  AttrLoc = AttrPrintLoc::Left;
else if (canPrintOnLeftSide(A)) {
  // For functions with body defined we print the attributes on the left
  // side so that GCC accept our dumps as well.
  if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D);
      FD && FD->isThisDeclarationADefinition())
    // In case Decl is a function with a body, then attrs should be print
    // on the left side.
    AttrLoc = AttrPrintLoc::Left;

    // In case it is a variable declaration with a ctor, then allow
    // printing on the left side for readbility.
  else if (const VarDecl *VD = dyn_cast<VarDecl>(D);
             VD && VD->getInit() &&
             VD->getInitStyle() == VarDecl::CallInit)
    AttrLoc = AttrPrintLoc::Left;
}
// Only print the side matches the user requested.
if ((Loc & AttrLoc) != AttrPrintLoc::None)
  A->printPretty(Out, Policy);

As you can see, both if (const FunctionDecl *FD and else if (const VarDecl *VD are inside the if (canPrintOnLeftSide()), and the AttrLoc variable is only set to AttrPrintLoc::Left when this function returns true. Hence unless there is a bug in canPrintOnLeftSide or mustPrintOnLeftSide (which I don't see any), I can't see a case where PrintOnLeft is false and it prints on the left side.

Now, I could be wrong on this, but IIRC the brackets on this is optional once the else if would match the preceding else-less if, which in this case would be if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D);, which is what the indentation meant.

[giulianobelinassi]

Fix dangling-else compilation warning.

[erichkeane]

clang/lib/AST/DeclPrinter.cpp

279

yep, you're right, it WAS already behaving how it was supposed to. Committed. Please keep an eye out for failed buildbots/etc.

[aaron.ballman]

clang/lib/AST/DeclPrinter.cpp

268–275

These changes have caused a new warning to show up in MSVC builds: switch statement contains 'default' but no 'case' labels

There's a few possible ways to resolve this: tablegen the entire switch statement (so you can elide the switch if there are no case statements and just make it return false; in that case), or remove the switch and add it back once the .inc file is no longer empty (this is a bit fragile for my tastes), or find an attribute that needs to be marked as must print on the left so the .inc file is no longer empty.

Can you look into solving this? I've not seen a bot go red from the change yet, but we do have warnings-as-error builds with MSVC (at least in downstreams) so the extra warning here is a problem.

[giulianobelinassi]

clang/lib/AST/DeclPrinter.cpp

268–275

tablegen the entire switch statement (so you can elide the switch if there are no case statements and just make it return false; in that case)

I will do this. This would IMHO give more control to the tool generating the table, which I think should have been the original approach. In any case, I think I will be able to come up with a patch in the next hours.

clang/lib/AST/DeclPrinter.cpp

class DeclPrinter : public DeclVisitor<DeclPrinter> {
     ///
     /// \param Quals The Objective-C declaration qualifiers.
     /// \param T The type to print.
     void PrintObjCMethodType(ASTContext &Ctx, Decl::ObjCDeclQualifier Quals,
                              QualType T);
 
     void PrintObjCTypeParams(ObjCTypeParamList *Params);
 
+    enum AttrPrintLoc {
+      None = 0,
+      Left = 1,
+      Middle = 2,
+      Right = 4,
+      Any = Left | Middle | Right,
+    };
+
+    void prettyPrintAttributes(Decl *D, raw_ostream &out,
+                               AttrPrintLoc loc = AttrPrintLoc::Any);
+
   public:
     DeclPrinter(raw_ostream &Out, const PrintingPolicy &Policy,
                 const ASTContext &Context, unsigned Indentation = 0,
                 bool PrintInstantiation = false)
         : Out(Out), Policy(Policy), Context(Context), Indentation(Indentation),
           PrintInstantiation(PrintInstantiation) {}
 
     void VisitDeclContext(DeclContext *DC, bool Indent = true);
public:
     void VisitHLSLBufferDecl(HLSLBufferDecl *D);
 
     void printTemplateParameters(const TemplateParameterList *Params,
                                  bool OmitTemplateKW = false);
     void printTemplateArguments(llvm::ArrayRef<TemplateArgument> Args,
                                 const TemplateParameterList *Params);
     void printTemplateArguments(llvm::ArrayRef<TemplateArgumentLoc> Args,
                                 const TemplateParameterList *Params);
-    void prettyPrintAttributes(Decl *D);
+
+    inline void prettyPrintAttributes(Decl *D) {
+      prettyPrintAttributes(D, Out);
+    }
+
     void prettyPrintPragmas(Decl *D);
     void printDeclType(QualType T, StringRef DeclName, bool Pack = false);
   };
 }
 
 void Decl::print(raw_ostream &Out, unsigned Indentation,
                  bool PrintInstantiation) const {
   print(Out, getASTContext().getPrintingPolicy(), Indentation, PrintInstantiation);

 }
 
 raw_ostream& DeclPrinter::Indent(unsigned Indentation) {
   for (unsigned i = 0; i != Indentation; ++i)
     Out << "  ";
   return Out;
 }
 
-void DeclPrinter::prettyPrintAttributes(Decl *D) {
+void DeclPrinter::prettyPrintAttributes(Decl *D, llvm::raw_ostream &Out,
+                                        AttrPrintLoc Loc) {
   if (Policy.PolishForDeclaration)
     return;
 
   if (D->hasAttrs()) {
     AttrVec &Attrs = D->getAttrs();
     for (auto *A : Attrs) {
       if (A->isInherited() || A->isImplicit())
         continue;
-      switch (A->getKind()) {
-#define ATTR(X)
-#define PRAGMA_SPELLING_ATTR(X) case attr::X:
-#include "clang/Basic/AttrList.inc"
-        break;
-      default:
+
+      // We will try to classify the attr::Kind into three disjoint sets:
+      // 1- should be print on the left side of a decl.
+      // 2- should be print on the middle of a decl right after the type.
+      // 3- should be print on the right side of a decl.
+      AttrPrintLoc attrloc = AttrPrintLoc::Right; // We default to right.
+      attr::Kind kind = A->getKind();
+
+      // FIXME: Find a way to use the AttrList.inc. We use if-else statements
+      // to classify each of them.
+      if (A->isStandardAttributeSyntax()) {
+        // C++11 onwards attributes can not be placed on left side. Output on
+        // right to mimic old clang behaviour.
+        attrloc = AttrPrintLoc::Right;
+      } else if (kind == attr::AsmLabel) {
+        // AsmLabels can not be placed on the middle nor left.
+        attrloc = AttrPrintLoc::Right;
+      } else if (kind == attr::DiagnoseIf || kind == attr::EnableIf) {
+        // DiagnoseIf should be print on the right side because it may refer to
+        // a variable that is declared as a parameter.
+        // FIXME: There are other attributes in which this is possible.
+        // Either parse the attribute to find if such variable is
+        // referenced or extend this to all these attributes.
+        attrloc = AttrPrintLoc::Right;
+      } else if (A->isDeclspecAttribute()) {
+        // Microsoft's __declspec should be print on the left side.
+        attrloc = AttrPrintLoc::Left;
+      } else if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D);
+                                          FD && FD->isThisDeclarationADefinition()) {
+        // In case Decl is a function with a body, then attrs should be print
+        // on the left side.
+        attrloc = AttrPrintLoc::Left;
+      }
+
+      // Only print the side matches the user requested.
+      if ((Loc & attrloc) != AttrPrintLoc::None) {
         A->printPretty(Out, Policy);
-        break;
       }
     }
   }
 }
 
 void DeclPrinter::prettyPrintPragmas(Decl *D) {
   if (Policy.PolishForDeclaration)
     return;
 
   if (D->hasAttrs()) {
     AttrVec &Attrs = D->getAttrs();
     for (auto *A : Attrs) {
       switch (A->getKind()) {
 #define ATTR(X)
 #define PRAGMA_SPELLING_ATTR(X) case attr::X:
 #include "clang/Basic/AttrList.inc"
         A->printPretty(Out, Policy);
         Indent();
         break;
       default:
void DeclPrinter::VisitFunctionDecl(FunctionDecl *D) {
   if (D->isFunctionTemplateSpecialization())
     Out << "template<> ";
   else if (!D->getDescribedFunctionTemplate()) {
     for (unsigned I = 0, NumTemplateParams = D->getNumTemplateParameterLists();
          I < NumTemplateParams; ++I)
       printTemplateParameters(D->getTemplateParameterList(I));
   }
 
+  std::string LeftsideAttrs;
+  llvm::raw_string_ostream LSAS(LeftsideAttrs);
+
+  prettyPrintAttributes(D, LSAS, AttrPrintLoc::Left);
+
+  // prettyPrintAttributes print a space on left side of the attribute.
+  if (LeftsideAttrs[0] == ' ') {
+    // Skip the space prettyPrintAttributes generated.
+    LeftsideAttrs.erase(0, LeftsideAttrs.find_first_not_of(' '));
+
+    // Add a single space between the attribute and the Decl name.
+    LSAS << ' ';
+  }
+
+  Out << LeftsideAttrs;
+
   CXXConstructorDecl *CDecl = dyn_cast<CXXConstructorDecl>(D);
   CXXConversionDecl *ConversionDecl = dyn_cast<CXXConversionDecl>(D);
   CXXDeductionGuideDecl *GuideDecl = dyn_cast<CXXDeductionGuideDecl>(D);
   if (!Policy.SuppressSpecifiers) {
     switch (D->getStorageClass()) {
     case SC_None: break;
     case SC_Extern: Out << "extern "; break;
     case SC_Static: Out << "static "; break;
     case SC_PrivateExtern: Out << "__private_extern__ "; break;
     case SC_Auto: case SC_Register:
if (Expr *TrailingRequiresClause = D->getTrailingRequiresClause()) {
       Out << " requires ";
       TrailingRequiresClause->printPretty(Out, nullptr, SubPolicy, Indentation,
                                           "\n", &Context);
     }
   } else {
     Ty.print(Out, Policy, Proto);
   }
 
-  prettyPrintAttributes(D);
+  prettyPrintAttributes(D, Out, Right);
 
   if (D->isPure())
     Out << " = 0";
   else if (D->isDeletedAsWritten())
     Out << " = delete";
   else if (D->isExplicitlyDefaulted())
     Out << " = default";
   else if (D->doesThisDeclarationHaveABody()) {

 
 void DeclPrinter::VisitLabelDecl(LabelDecl *D) {
   Out << *D << ":";
 }
 
 void DeclPrinter::VisitVarDecl(VarDecl *D) {
   prettyPrintPragmas(D);
 
+  std::string Declspec;
+  llvm::raw_string_ostream DeclspecStream(Declspec);
+
+  // Print attributes that should be placed on the left, such as __declspec.
+  prettyPrintAttributes(D, DeclspecStream, AttrPrintLoc::Left);
+
+  // prettyPrintAttributes print a space on left side of the attribute.
+  if (Declspec[0] == ' ') {
+    // Skip the space prettyPrintAttributes generated.
+    Declspec.erase(0, Declspec.find_first_not_of(' '));
+
+    // Add a single space between the attribute and the Decl name.
+    DeclspecStream << ' ';
+  }
+
+  Out << Declspec;
+
   QualType T = D->getTypeSourceInfo()
     ? D->getTypeSourceInfo()->getType()
     : D->getASTContext().getUnqualifiedObjCPointerType(D->getType());
 
   if (!Policy.SuppressSpecifiers) {
     StorageClass SC = D->getStorageClass();
     if (SC != SC_None)
       Out << VarDecl::getStorageClassSpecifierString(SC) << " ";
if (D->isModulePrivate())
       Out << "__module_private__ ";
 
     if (D->isConstexpr()) {
       Out << "constexpr ";
       T.removeLocalConst();
     }
   }
 
-  printDeclType(T, (isa<ParmVarDecl>(D) && Policy.CleanUglifiedParameters &&
-                    D->getIdentifier())
-                       ? D->getIdentifier()->deuglifiedName()
-                       : D->getName());
+  std::string Name;
+
+  Name += (isa<ParmVarDecl>(D) && Policy.CleanUglifiedParameters &&
+              ? D->getIdentifier()->deuglifiedName().str()
+              : D->getName().str();
+
+  printDeclType(T, StringRef(Name));
+
+  // Print the attributes that should be placed right before the end of the
+  // decl.
+  prettyPrintAttributes(D, Out, Right);
+
   Expr *Init = D->getInit();
   if (!Policy.SuppressInitializers && Init) {
     bool ImplicitInit = false;
     if (D->isCXXForRangeDecl()) {
       // FIXME: We should print the range expression instead.
       ImplicitInit = true;
     } else if (CXXConstructExpr *Construct =
                    dyn_cast<CXXConstructExpr>(Init->IgnoreImplicit())) {
if (!ImplicitInit) {
       PrintingPolicy SubPolicy(Policy);
       SubPolicy.SuppressSpecifiers = false;
       SubPolicy.IncludeTagDefinition = false;
       Init->printPretty(Out, nullptr, SubPolicy, Indentation, "\n", &Context);
       if ((D->getInitStyle() == VarDecl::CallInit) && !isa<ParenListExpr>(Init))
         Out << ")";
     }
   }
-  prettyPrintAttributes(D);
 }
 
 void DeclPrinter::VisitParmVarDecl(ParmVarDecl *D) {
   VisitVarDecl(D);
 }
 
 void DeclPrinter::VisitFileScopeAsmDecl(FileScopeAsmDecl *D) {
   Out << "__asm (";

clang/test/AST/ast-print-attr-knr.c

+// This file contain tests for attribute arguments on K&R functions.
+
+// RUN: %clang_cc1 -ast-print -x c -std=c89 -fms-extensions %s -o - | FileCheck %s
+
+// CHECK: int knr(i)
+// CHECK-NEXT: int i __attribute__((unused));
+// CHECK-NEXT: {
+// CHECK-NEXT: return 0;
+// CHECK-NEXT: }
+int knr(i) int i __attribute__((unused)); { return 0; }
+
+// CHECK: __attribute__((unused)) int knr2(i)
+// CHECK-NEXT: int i;
+// CHECK-NEXT: {
+// CHECK-NEXT: return 0;
+// CHECK-NEXT: }
+__attribute__((unused)) int knr2(i) int i; { return 0; }

clang/test/AST/ast-print-attr.c

enum __attribute__((ns_error_domain(MyErrorDomain))) MyErrorEnum {
   MyErrSecond,
 };
 
 // CHECK: int *fun_returns() __attribute__((ownership_returns(fun_returns)));
 int *fun_returns() __attribute__((ownership_returns(fun_returns)));
 
 // CHECK: void fun_holds(int *a) __attribute__((ownership_holds(fun_holds, 1)));
 void fun_holds(int *a) __attribute__((ownership_holds(fun_holds, 1)));
+
+// CHECK: int fun_var_unused() {
+// CHECK-NEXT: int x __attribute__((unused)) = 0;
+// CHECK-NEXT: return x;
+// CHECK-NEXT: }
+int fun_var_unused() { int x __attribute__((unused)) = 0; return x; }

clang/test/AST/ast-print-pragmas.cpp

 
 void test_templates(int *List, int Length) {
   test_nontype_template_param<2, 4>(List, Length);
 }
 
 #ifdef MS_EXT
 #pragma init_seg(compiler)
 // MS-EXT: #pragma init_seg (.CRT$XCC){{$}}
-// MS-EXT-NEXT: int x = 3 __declspec(thread);
-int __declspec(thread) x = 3;
+// MS-EXT-NEXT: __declspec(thread) int x = 3;
+__declspec(thread) int x = 3;
 #endif //MS_EXT
 

clang/test/Analysis/blocks.mm

 void testBlockWithCaptureByReference() {
   __block StructWithCopyConstructor s(5);
   // Captures s by reference, so no copy expression.
   (void)(^void() {
     (void)s;
   });
 }
 
 // CHECK-LABEL:void testBlockWithCaptureByReference()
 // CHECK-NEXT: [B2 (ENTRY)]
 // CHECK-NEXT:   Succs (1): B1
 
 // CHECK: [B1]
 // CHECK-NEXT:   1: 5
 // WARNINGS-NEXT:   2: [B1.1] (CXXConstructExpr, StructWithCopyConstructor)
 // ANALYZER-NEXT:   2: [B1.1] (CXXConstructExpr, [B1.3], StructWithCopyConstructor)
-// CHECK-NEXT:   3: StructWithCopyConstructor s(5) __attribute__((blocks("byref")));
+// CHECK-NEXT:   3: StructWithCopyConstructor s __attribute__((blocks("byref")))(5);
 // CHECK-NEXT:   4: ^{ }
 // CHECK-NEXT:   5: (void)([B1.4]) (CStyleCastExpr, ToVoid, void)
 // CHECK-NEXT:   Preds (1): B2
 // CHECK-NEXT:   Succs (1): B0
 
 // CHECK: [B0 (EXIT)]
 // CHECK-NEXT:   Preds (1): B1

clang/test/OpenMP/assumes_codegen.cpp

 
 #pragma omp begin assumes ext_lambda_assumption
 int lambda_outer() {
   auto lambda_inner = []() { return 42; };
   return lambda_inner();
 }
 #pragma omp end assumes
 
-// AST:      void foo() __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) {
+// AST:      __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) void foo() {
 // AST-NEXT: }
 // AST-NEXT: class BAR {
 // AST-NEXT: public:
-// AST-NEXT:     BAR() __attribute__((assume("ompx_range_bar_only"))) __attribute__((assume("ompx_range_bar_only_2"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp")))     {
+// AST-NEXT:     __attribute__((assume("ompx_range_bar_only"))) __attribute__((assume("ompx_range_bar_only_2"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) BAR()      {
 // AST-NEXT:     }
-// AST-NEXT:     void bar1() __attribute__((assume("ompx_range_bar_only"))) __attribute__((assume("ompx_range_bar_only_2"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp")))     {
+// AST-NEXT:     __attribute__((assume("ompx_range_bar_only"))) __attribute__((assume("ompx_range_bar_only_2"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) void bar1()     {
 // AST-NEXT:     }
-// AST-NEXT:     static void bar2() __attribute__((assume("ompx_range_bar_only"))) __attribute__((assume("ompx_range_bar_only_2"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp")))     {
+// AST-NEXT:     __attribute__((assume("ompx_range_bar_only"))) __attribute__((assume("ompx_range_bar_only_2"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) static void bar2()      {
 // AST-NEXT:     }
 // AST-NEXT: };
-// AST-NEXT: void bar() __attribute__((assume("ompx_range_bar_only"))) __attribute__((assume("ompx_range_bar_only_2"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) {
+// AST-NEXT:  __attribute__((assume("ompx_range_bar_only"))) __attribute__((assume("ompx_range_bar_only_2"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) void bar() {
 // AST-NEXT:     BAR b;
 // AST-NEXT: }
 // AST-NEXT: void baz() __attribute__((assume("ompx_1234"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp")));
 // AST-NEXT: template <typename T> class BAZ {
 // AST-NEXT: public:
-// AST-NEXT:     BAZ<T>() __attribute__((assume("ompx_1234"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp")))     {
+// AST-NEXT:     __attribute__((assume("ompx_1234"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) BAZ<T>()      {
 // AST-NEXT:     }
-// AST-NEXT:     void baz1() __attribute__((assume("ompx_1234"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp")))     {
+// AST-NEXT:     __attribute__((assume("ompx_1234"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp")))  void baz1()     {
 // AST-NEXT:     }
-// AST-NEXT:     static void baz2() __attribute__((assume("ompx_1234"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp")))     {
+// AST-NEXT:     __attribute__((assume("ompx_1234"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) static void baz2()      {
 // AST-NEXT:     }
 // AST-NEXT: };
 // AST-NEXT: template<> class BAZ<float> {
 // AST-NEXT: public:
-// AST-NEXT:     BAZ() __attribute__((assume("ompx_1234"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp")))    {
+// AST-NEXT:     __attribute__((assume("ompx_1234"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp")))  BAZ()    {
 // AST-NEXT:     }
 // AST-NEXT:     void baz1() __attribute__((assume("ompx_1234"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp")));
 // AST-NEXT:     static void baz2() __attribute__((assume("ompx_1234"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp")));
 // AST-NEXT: };
-// AST-NEXT: void baz() __attribute__((assume("ompx_1234"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) {
+// AST-NEXT: __attribute__((assume("ompx_1234"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) void baz() {
 // AST-NEXT:     BAZ<float> b;
 // AST-NEXT: }
-// AST-NEXT: int lambda_outer() __attribute__((assume("ompx_lambda_assumption"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) {
+// AST-NEXT: __attribute__((assume("ompx_lambda_assumption"))) __attribute__((assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses"))) __attribute__((assume("omp_no_openmp"))) int lambda_outer() {
 // AST-NEXT:     auto lambda_inner = []() {
 // AST-NEXT:         return 42;
 // AST-NEXT:     };
 // AST-NEXT:     return lambda_inner();
 // AST-NEXT: }
 
 #endif
 

clang/test/OpenMP/assumes_print.cpp

 #pragma omp begin assumes ext_not_seen
 #pragma omp end assumes
 
 #pragma omp begin assumes ext_1234
 void baz() {
 }
 #pragma omp end assumes
 
-// CHECK: void foo() __attribute__((assume("omp_no_openmp_routines"))) __attribute__((assume("omp_no_openmp")))
-// CHECK: void bar() __attribute__((assume("ompx_range_bar_only"))) __attribute__((assume("ompx_range_bar_only_2"))) __attribute__((assume("omp_no_openmp_routines"))) __attribute__((assume("omp_no_openmp")))
-// CHECK: void baz() __attribute__((assume("ompx_1234"))) __attribute__((assume("omp_no_openmp_routines"))) __attribute__((assume("omp_no_openmp")))
+// CHECK: __attribute__((assume("omp_no_openmp_routines"))) __attribute__((assume("omp_no_openmp"))) void foo()
+// CHECK: __attribute__((assume("ompx_range_bar_only"))) __attribute__((assume("ompx_range_bar_only_2"))) __attribute__((assume("omp_no_openmp_routines"))) __attribute__((assume("omp_no_openmp"))) void bar()
+// CHECK: __attribute__((assume("ompx_1234"))) __attribute__((assume("omp_no_openmp_routines"))) __attribute__((assume("omp_no_openmp"))) void baz()
 
 #endif

clang/test/OpenMP/assumes_template_print.cpp

 
 #ifndef HEADER
 #define HEADER
 
 template <typename T>
 struct S {
   int a;
 // CHECK: template <typename T> struct S {
-// CHECK:     void foo() __attribute__((assume("ompx_global_assumption")))     {
+// CHECK:     __attribute__((assume("ompx_global_assumption"))) void foo()     {
   void foo() {
     #pragma omp parallel
     {}
   }
 };
 
 // CHECK: template<> struct S<int> {
-// CHECK:     void foo() __attribute__((assume("ompx_global_assumption")))     {
+// CHECK:     __attribute__((assume("ompx_global_assumption"))) void foo()     {
 
 #pragma omp begin assumes no_openmp
-// CHECK: void S_with_assumes_no_call() __attribute__((assume("omp_no_openmp"))) __attribute__((assume("ompx_global_assumption"))) {
+// CHECK: __attribute__((assume("omp_no_openmp"))) __attribute__((assume("ompx_global_assumption"))) void S_with_assumes_no_call() {
 void S_with_assumes_no_call() {
   S<int> s;
   s.a = 0;
 }
-// CHECK: void S_with_assumes_call() __attribute__((assume("omp_no_openmp"))) __attribute__((assume("ompx_global_assumption"))) {
+// CHECK: __attribute__((assume("omp_no_openmp"))) __attribute__((assume("ompx_global_assumption"))) void S_with_assumes_call() {
 void S_with_assumes_call() {
   S<int> s;
   s.a = 0;
   // If this is executed we have UB!
   s.foo();
 }
 #pragma omp end assumes
 
-// CHECK: void S_without_assumes() __attribute__((assume("ompx_global_assumption"))) {
+// CHECK: __attribute__((assume("ompx_global_assumption"))) void S_without_assumes() {
 void S_without_assumes() {
   S<int> s;
   s.foo();
 }
 
 #pragma omp assumes ext_global_assumption
 
 // Same as the struct S above but the order in which we instantiate P is different, first outside of an assumes.
 template <typename T>
 struct P {
 // CHECK: template <typename T> struct P {
-// CHECK:     void foo() __attribute__((assume("ompx_global_assumption")))     {
+// CHECK:    __attribute__((assume("ompx_global_assumption"))) void foo()      {
   int a;
   void foo() {
     #pragma omp parallel
     {}
   }
 };
 
 // TODO: Avoid the duplication here:
 
 // CHECK: template<> struct P<int> {
-// CHECK:     void foo() __attribute__((assume("ompx_global_assumption"))) __attribute__((assume("ompx_global_assumption")))     {
+// CHECK:      __attribute__((assume("ompx_global_assumption"))) __attribute__((assume("ompx_global_assumption"))) void foo()   {
 
-// CHECK: void P_without_assumes() __attribute__((assume("ompx_global_assumption"))) {
+// CHECK: __attribute__((assume("ompx_global_assumption"))) void P_without_assumes() {
 void P_without_assumes() {
   P<int> p;
   p.foo();
 }
 
 #pragma omp begin assumes no_openmp
-// CHECK: void P_with_assumes_no_call() __attribute__((assume("omp_no_openmp"))) __attribute__((assume("ompx_global_assumption"))) {
+// CHECK: __attribute__((assume("omp_no_openmp"))) __attribute__((assume("ompx_global_assumption"))) void P_with_assumes_no_call() {
 void P_with_assumes_no_call() {
   P<int> p;
   p.a = 0;
 }
-// CHECK: void P_with_assumes_call() __attribute__((assume("omp_no_openmp"))) __attribute__((assume("ompx_global_assumption"))) {
+// CHECK: __attribute__((assume("omp_no_openmp"))) __attribute__((assume("ompx_global_assumption"))) void P_with_assumes_call() {
 void P_with_assumes_call() {
   P<int> p;
   p.a = 0;
   // If this is executed we have UB!
   p.foo();
 }
 #pragma omp end assumes
 
 #endif

clang/test/OpenMP/declare_simd_ast_print.cpp

void h(int *hp, int *hp2, int *hq, int *lin)
   // Implicit specialization with <C=float>.
   // This is special case where the directive is stored by Sema and is
   // generated together with the (pending) function instatiation.
   h((float*) hp, (float*) hp2, (float*) hq, (float*) lin);
 }
 
 class VV {
   // CHECK: #pragma omp declare simd uniform(this, a) linear(val(b): a)
-  // CHECK-NEXT: int add(int a, int b) __attribute__((cold))    {
+  // CHECK-NEXT:  __attribute__((cold)) int add(int a, int b)     {
   // CHECK-NEXT: return a + b;
   // CHECK-NEXT: }
   #pragma omp declare simd uniform(this, a) linear(val(b): a)
-  int add(int a, int b) __attribute__((cold)) { return a + b; }
+  __attribute__((cold)) int add(int a, int b) { return a + b; }
 
   // CHECK: #pragma omp declare simd aligned(b: 4) aligned(a) linear(ref(b): 4) linear(val(this)) linear(val(a))
   // CHECK-NEXT: float taddpf(float *a, float *&b)     {
   // CHECK-NEXT: return *a + *b;
   // CHECK-NEXT: }
   #pragma omp declare simd aligned (b: 4) aligned(a) linear(ref(b): 4) linear(this, a)
   float taddpf(float *a, float *&b) { return *a + *b; }
 

clang/test/Sema/attr-print.c

 // RUN: %clang_cc1 %s -ast-print -fms-extensions | FileCheck %s
 
 // CHECK: int x __attribute__((aligned(4)));
 int x __attribute__((aligned(4)));
 
-// FIXME: Print this at a valid location for a __declspec attr.
-// CHECK: int y __declspec(align(4));
+// CHECK: __declspec(align(4)) int y;
 __declspec(align(4)) int y;
 
 // CHECK: short arr[3] __attribute__((aligned));
 short arr[3] __attribute__((aligned));
 
 // CHECK: void foo(void) __attribute__((const));
 void foo(void) __attribute__((const));
 
 // CHECK: void bar(void) __attribute__((__const));
 void bar(void) __attribute__((__const));
 
 // CHECK: int * __ptr32 p32;

clang/test/SemaCXX/attr-print.cpp

 // RUN: %clang_cc1 %s -ast-print -fms-extensions | FileCheck %s
 
 // CHECK: int x __attribute__((aligned(4)));
 int x __attribute__((aligned(4)));
 
-// FIXME: Print this at a valid location for a __declspec attr.
-// CHECK: int y __declspec(align(4));
+// CHECK: __declspec(align(4)) int y;
 __declspec(align(4)) int y;
 
 // CHECK: void foo() __attribute__((const));
 void foo() __attribute__((const));
 
 // CHECK: void bar() __attribute__((__const));
 void bar() __attribute__((__const));
 

clang/test/SemaCXX/cxx11-attr-print.cpp

 // RUN: %clang_cc1 -std=c++11 -ast-print -fms-extensions %s | FileCheck %s
 //
 // CHECK: int x __attribute__((aligned(4)));
 int x __attribute__((aligned(4)));
 
-// FIXME: Print this at a valid location for a __declspec attr.
-// CHECK: int y __declspec(align(4));
+// CHECK: __declspec(align(4)) int y;
 __declspec(align(4)) int y;
 
 // CHECK: int z {{\[}}[gnu::aligned(4)]];
 int z [[gnu::aligned(4)]];
 
 // CHECK: __attribute__((deprecated("warning")));
 int a __attribute__((deprecated("warning")));
 

 inline void f7 [[gnu::gnu_inline]] ();
 
 // arguments printing
 // CHECK: __attribute__((format(printf, 2, 3)));
 void f8 (void *, const char *, ...) __attribute__ ((format (printf, 2, 3)));
 
 // CHECK: int m __attribute__((aligned(4
 // CHECK: int n alignas(4
-// CHECK: static int f() __attribute__((pure))
+// CHECK: __attribute__((pure)) static int f()
 // CHECK: static int g() {{\[}}[gnu::pure]]
 template <typename T> struct S {
   __attribute__((aligned(4))) int m;
   alignas(4) int n;
   __attribute__((pure)) static int f() {
     return 0;
   }
   [[gnu::pure]] static int g() {
     return 1;
   }
 };
 
 // CHECK: int m __attribute__((aligned(4
 // CHECK: int n alignas(4
-// CHECK: static int f() __attribute__((pure))
+// CHECK: __attribute__((pure)) static int f()
 // CHECK: static int g() {{\[}}[gnu::pure]]
 template struct S<int>;
 
 // CHECK: using Small2 {{\[}}[gnu::mode(byte)]] = int;
 using Small2 [[gnu::mode(byte)]] = int;