[PATCH] D141714: Fix ast print of variables with attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D141714#4175942 <https://reviews.llvm.org/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;


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141714/new/

https://reviews.llvm.org/D141714