In D61717#1668980, @SjoerdMeijer wrote:

Do you perhaps have a test case or error that I can look at? Perhaps I or someone else here can help out a bit here.

In D67399#1668759, @rjmccall wrote:

The exact sequence of volatile accesses is observable behavior, and it's the ABI's right to define correct behavior for compliant implementations, so we do need to do this.

This patch doesn't work, and the ARM NEON intrinsic header emitter is too opaque and mysterious for me to be able to fix this, and it's not even clear that the generator knows what the actual parameter types of the compiler builtins are. For now I think the best I can do is to leave ARM targets with lax vector conversions enabled, and disable it for all the other targets. :-(

In D67414#1668475, @mstorsjo wrote:

In D67414#1668443, @rsmith wrote:

Seems very surprising to me that the gnu_inline attribute has different behavior in their C and C++ frontends. That said, it looks like it's a behavior that they document; their documentation says "In C++, this attribute does not depend on extern in any way, but it still requires the inline keyword to enable its special behavior." and that matches their observed behavior. And they behave this way even for functions in extern "C" blocks. The question for us, then, is do we want to be compatible with C source code built as C++ (that is, the status quo) and C++ source code using this attribute and targeting older versions of Clang, or with g++?

I think it's a rather uncommon combination (I think most use of gnu_inline is together with extern), so I wouldn't think that there's a lot of users relying on the current behaviour (in clang-only codebases), but I have no data to back it up with.

Is Clang's current behavior actually causing problems for some real use case? Changing this behavior is not without risk, and the old behavior is certainly defensible.

It did come up in mingw-w64 recently; new header additions (that were developed with gcc) broke when built with clang (due to multiple definitions of a symbol). It's been rectified for now by avoiding this combination (for compatibility with existing clang versions) though.

Seems very surprising to me that the gnu_inline attribute has different behavior in their C and C++ frontends. That said, it looks like it's a behavior that they document; their documentation says "In C++, this attribute does not depend on extern in any way, but it still requires the inline keyword to enable its special behavior." and that matches their observed behavior. And they behave this way even for functions in extern "C" blocks. The question for us, then, is do we want to be compatible with C source code built as C++ (that is, the status quo) and C++ source code using this attribute and targeting older versions of Clang, or with g++?

Just one thing I missed before, otherwise this looks good. Thanks!

In D59754#1656217, @leonardchan wrote:

Hi! We've noticed that for our arm bots, we're getting some flaky builds that sometimes fail with error: array designators are a C99 extension [-Werror,-Wc99-designator] and sometimes don't fail. 2 questions:

1. I can't see it off the patch immediately, but do you know why for arm specifically we can only get this warning sometimes?

In D66361#1655903, @krytarowski wrote:

This change broke on NetBSD.

http://lab.llvm.org:8011/builders/netbsd-amd64/builds/22003/steps/run%20unit%20tests/logs/FAIL%3A%20Clang%3A%3Astack-exhaustion.cpp

I'm taking this over to finish it off and submit.

In D64799#1608085, @ilya-biryukov wrote:

@rsmith two options for this patch seem to be:

• silently ignore the errors (behavior before this patch),
• show them to the user at the end of TU (what happens in the current version of the patch). This can result in bad UX, but we won't drop any errors on the floor. Which one do you think we should prefer?

LGTM: while this isn't a solution to the root cause of the issues here, it puts us in a better situation than the status quo.

Do we need KnownModules at all? It seems to serve a very similar purpose to the Modules string map on ModuleMap. (The main difference seems to be that KnownModules can cache module load failures.) If we can keep only a single data structure tracking this, owned by the ModuleMap (which also owns the modules), that should remove the possibility for dangling module pointers.

This isn't the right approach: instead of re-instantiating the parameter when we encounter a use of it, we should inject the parameters into the local instantiation scope before trying to instantiate a default argument (Sema::CheckCXXDefaultArgExpr should call addInstantiatedParametersToScope to populate the LocalInstantiationScope before calling SubstInitializer).

I think we're past the point of large-scale structural comments that are better addressed before the first commit, and it'd be much better to make incremental improvements from here.

In D66364#1638026, @aaron.ballman wrote:

@rsmith are you fine with implementing the diagnostic for these keywords piecemeal based on the pattern from this patch, or do you want to see an omnibus patch that adds all of them at once?

This seems subtle, but I believe it is correct.

• Address review comments from Aaron.

Ping. Is there any interest in pursuing this, or is the preprocessor complexity too great?

The ALPHA_OFFSET code seems to be unnecessarily "clever" to me. I think the warning can be suppressed by reversing the operands:

In D66364#1633981, @aaron.ballman wrote:

My motivation is for portability. _Thread_local (and all the rest) do not exist in C99 or earlier (or C++), so having some way to warn users of that is useful. I agree that we should be consistent and go with all or none, but my preference is for all (esp since this is a -pedantic warning class).

_Thread_local is a reserved identifier; we generally don't produce extension warnings for uses of reserved identifiers. (Eg, there's no warning for _Atomic in C++ or _Bool in C89, and no warning for uses of __type_traits or __builtins.)

• Disable stack exhaustion test if threads are disabled.

• Review feedback: use _AddressOfReturnAddress with MSVC, and simplify

LGTM with minor adjustments to the test.

I would like to better understand the big picture for descriptors, pointers, and so on. I'm not yet seeing how the pieces are going to fit together and not frequently require expensive operations. For example, pointer arithmetic requires determining the array bound of the pointee; pointer subtraction requires determining whether two pointers point into the same array; pointer comparisons require determining the divergence point in the access paths between two pointers. These path-based operations were easy in the old representation, and seem to be difficult in the new representation, so I want to make sure that they've been properly considered. It's also not clear to me how you'll model pointers that have lost their designator (through constant folding), where you have a runtime offset but no compile-time offset, or pointers that point to objects whose values are not part of the evaluation (for example, extern globals), or pointers that have a designator but no base (for __builtin_object_size handling).