Ping.

Aside from some small nits, LGTM!

Currently the smart_ptr check (modernize-make-unique) generates the fixes that cannot compile for cases like below -- because brace list can not be deduced in make_unique.

class Bar { int a, b; };
class Foo { Foo(Bar); };
auto foo = std::unique_ptr<Foo>(new Foo({1, 2}));

Sorry about the delayed review on this -- I had intended to provide comments earlier, but somehow this fell off my radar.

I've commit in r347163. Thank you for the patch!

It looks like you removed a considerable amount of testing coverage; why?

In D54246#1301940, @hwright wrote:

@aaron.ballman I don't actually have the commit bit, can you commit this, or are we waiting for further review?

In D54555#1301748, @chandlerc wrote:

In D54555#1301091, @aaron.ballman wrote:

Would there be value in making this a type attribute that appertains to the function type so that you don't lose this protection when calling through function pointers (as you could then add the attribute to the function pointer type as well)? If the stack pointer value is changed as part of the function prologue, this may not be needed, but if the stack pointer value is changed on the calling side, perhaps this would add value?

IMO, no.

This is fundamentally a property of the definition of a function, not of a call of a function. Whether the function being called has SLH or not doesn't change how you call it in any way.

I think this check is in good shape for the initial commit. Additional functionality can be added incrementally.

LGTM!

In D54555#1301087, @kristof.beyls wrote:

In D54555#1301084, @aaron.ballman wrote:

Does this hardening impact the ABI in any way? e.g., do we have to do anything special to handle calls through function pointers where the bound function pointer is marked with this attribute?

Not entirely sure, but I don't think so.
The way this is implemented both for x86 (if I understood the code correctly) and how this most likely will be implemented for AArch64 (see https://reviews.llvm.org/D49069 for what I expect will become the basis to implement SLH for AArch64) is that an illegal value is put in the stack pointer to signal misspeculation having happened. So, in that respect, having that illegal value in the stack pointer is an ABI rule to communicate mis-speculation across function boundaries.
However, given that this is ABI only used on miss-speculated paths, and miss-speculated paths get corrected eventually, programs will keep on working correctly if functions calling each other don't follow the same ABI in this respect. Obviously, you'll lose some protection against cross-function call miss-speculation.

Does this hardening impact the ABI in any way? e.g., do we have to do anything special to handle calls through function pointers where the bound function pointer is marked with this attribute?

I'm not certain if it will be possible to devise test cases for the two diagnostics I pointed out or not, but otherwise, this LGTM as far as the implementation goes. I don't know enough about AVR to sign off on whether the patch logic is correct or not.

Missing sema tests that demonstrate the attribute can only appertain to functions and types, accepts no arguments, has the expected semantic behavior for typecasts to function pointers, etc.

In D54547#1299105, @chandlerc wrote:

Should likely add release notes about this.

Also might be worth sending a note to cfe-dev as a heads up and give folks some time to say "wait wait".

In D54258#1297687, @richardmembarth wrote:

In D54258#1297191, @Anastasia wrote:

I agree with the change itself... but it's quite annoying that such things can't be tested. :(

Yes, that's a pity :(

In D54450#1298319, @vmiklos wrote:

Oh, and running the check-clang-tools target, this currently results in:

Failing Tests (3):
    Clang Tools :: modularize/ProblemsInconsistent.modularize
    Clang Tools :: pp-trace/pp-trace-conditional.cpp
    Clang Tools :: pp-trace/pp-trace-macro.cpp

Perhaps the pp-trace one just has to be updated for the new behavior. :-)

Updating based on review feedback.

In D52835#1298428, @xbolva00 wrote:

Reverted.
Ok, I will address your comments soon.

In D51575#1297664, @stephanemoore wrote:

Thanks for the review everyone!

Let me know if there are any further changes that you want me to make or any further action required on my part to land this 😁

I'll go ahead and add myself as code owner. Peter can remove himself at his leisure.

In D54258#1295158, @richardmembarth wrote:

There are external tools (e.g. hipacc) that generate Clang AST. This AST uses LangAS annotations and emits incorrect memory space specifiers for CUDA when pretty-printed.

In D54402#1296273, @steveire wrote:

This is just a NFC change, which is normal to appear without tests. The consensus on IRC is that this is fine.

Thank you for all of your hard work on clang-query!

Aside from a minor formatting nit, LGTM.

In D54407#1295555, @sbenza wrote:

In D54407#1294934, @aaron.ballman wrote:

Adding @sbenza in case he has opinions on this approach. I think it's reasonable, but I also know that changes to the the AST matcher internals sometimes have unintended side effects with the various instantiations.

The problem used to come with the number of template instantiations, but afaik this was with an MSVC configuration that is no longer supported.
In any case, I don't see this change making new template instantiations.

LGTM!

Aside from the uses of auto and the lack of tests, this LGTM.

In D54404#1295426, @steveire wrote:

I acknowledge and share the future-proofing concern.

We could possibly use something trait-based instead and put the trait beside the matcher definition in ASTMatchers.h, but that doesn't really solve the problem. It only moves the problem.

LGTM aside from a minor commenting nit.

In D54402#1295420, @steveire wrote:

I think this commit is fine without tests.

Adding @sbenza in case he has opinions on this approach. I think it's reasonable, but I also know that changes to the the AST matcher internals sometimes have unintended side effects with the various instantiations.

Generally LGTM but this has no test coverage. What are your plans for how you want to see this proceed? Do you intend to commit everything in one batch once all of the reviews are accepted, or do piecemeal commits?

Generally LGTM, but I leave it to someone more well-versed in ObjC to sign off that this actually does everything those users would expect.

LGTM aside from some small commenting nits. However, I leave it to @erik.pilkington to make the final sign-off.

In D54349#1294325, @vmiklos wrote:

What do you think about code like:

#if FOO == 4
#if FOO == 4
#endif
#endif

#if defined(FOO)
#if defined(FOO)
#endif
#endif

#if !defined(FOO)
#if !defined(FOO)
#endif
#endif

I looked at supporting these, but it's not clear to me how to get e.g. the 'FOO == 4' string (the condition) from the If() callback. So far I only see how to get the start location of the condition and the whole range till the end of endif. Do you have a code pointer how to do this, please? (Or OK if I leave this for a follow-up patch?)

In D52984#1294223, @xazax.hun wrote:

• Move the checklist up before additional info in the HTML file.
• Fix minor nits.
• Add missing bullet points (thanks @Szelethus for noticing)
  
  I did not add any coding convention related item yet. I wonder if it is a good idea to have our own coding guidelines even if it is derived from the LLVM one.

What do you think about code like:

#if FOO == 4
#if FOO == 4
#endif
#endif

In D54344#1293468, @erik.pilkington wrote:

Have you tried running creduce on the preprocessed source? We should really have a real reproducer for this, otherwise its really hard to tell what the underlying problem is.

LGTM!

Adding a few other reviewers in case they have ideas on how to use the polymorphic matcher rather than the overload. If they don't have a better idea, then I think the overload approach is fine.

In D54258#1292129, @richardmembarth wrote:

I think it's not so easy to provide such tests for CUDA.
CUDA memory space specifiers are implemented via attributes, e.g. #define __shared__ __attribute__((shared)).
As a result of this, they are pretty-printed via a different code path.
In my example, I call Ctx.getAddrSpaceQualType(QT, LangAS::cuda_shared), which is then pretty-printed via the code above.
Any hints how to provide tests for this one?

Thanks for the reviews -- I've commit in r346521.

LGTM aside from a minor commenting nit.

Also: the patch is missing unit tests.

Can you add tests for this change? We typically have these in Misc by passing -ast-print.

In D52421#1288910, @lebedev.ri wrote:

And now i'm having concerns.

struct Base {
    void* f;
};

struct Inherit : Base {
    static void func(void* f) { // <- does 'f' *actually* shadow the 'f' in the 'Base'? You can't access that non-static member variable from static function.
    }
};

Another option that @rsmith and I discussed today is perhaps using __clang or clang__ as the identifier, but perhaps this will cause more confusion about where to put underscores than _Clang would. If there are other ideas that are more palatable for the identifier, that'd be great -- I am not strongly tied to a spelling.

I've commit in r346041.

Thank you for the quick review and feedback -- I've commit in r345874.