In D147655#4650666, @rjmccall wrote:

Yeah, the more I think about this, the more I think that while (1) Apple should upstream its use of an older default, regardless (2) the existence of any targets at all with an older default means that tests like this always need to be using -fclang-abi-compat=latest.

In D147655#4649864, @dyung wrote:

Hi @rsmith, we have an internal test where your change seems to have changed the mangling in C++17 mode and wanted to check if that was intentional.

[...]

Are these changes in non-C++20 mode intentional?

• Merge

The changes in SemaInit.cpp don't look correct to me. Trying to recurse through the initializer and find every nested temporary is highly error-prone; I can't tell you which expression nodes you forgot to recurse through, but I'm sure there are some.

Ping. Are there any further concerns here? (This obviously needs to be merged with trunk, and the -fclang-abi-compat= checks and release notes need to be updated to match the latest release version.)

In D158967#4621361, @zyounan wrote:

Currently the code in CompilerInstance::ExecuteAction seems to acknowledge that there should be a fallback. I'm suggesting to move this fallback down to a function that actually runs parsing.

One thing I'm afraid of is, that there are/were some compatible reasons that made us decide not to insert this call at ASTFrontendAction::ExecuteAction nor clang::ParseAST. (I didn't see the explanation in D66361. Richard, could you kindly explain why? @rsmith)

In D155387#4579866, @ilya-biryukov wrote:

I believe this should compile as according to (over.match.oper)p4:

A non-template function or function template F named operator== is a rewrite target with first operand o unless a search for the name operator!= in the scope S from the instantiation context of the operator expression finds a function …

And 'search' is defined without any checks for ambiguous bases in (basic.lookup.general)p3

A single search in a scope S for a name N from a program point P finds all declarations that precede P to which any name that is the same as N ([basic.pre]) is bound in S. If any such declaration is a using-declarator whose terminal name ([expr.prim.id.unqual]) is not dependent ([temp.dep.type]), it is replaced by the declarations named by the using-declarator ([namespace.udecl]).

Clang does a full lookup instead, therefore exposing the errors that it should not diagnose.

The prior behavior of Clang is correct. A search of a class scope is ill-formed if it finds an ambiguous result, see http://eel.is/c++draft/basic.lookup#class.member.lookup-6.sentence-2

Clang was correct here until fairly recently; P2280 (https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2280r4.html) changed the language rules. It was applied as a DR, so we should make that change retroactively rather than only in C++23 mode. See https://github.com/llvm/llvm-project/issues/63139, which tracks implementation of that language change.

Awesome, thanks! It makes perfect sense that rG61c7a9140b would fix this.

Looks good, thanks!

This looks good to me. I think we can further refine the handling of duplicate diagnostics but I don't think that needs to be done as part of this bugfix.

In D156210#4531000, @ChuanqiXu wrote:

From what @rsmith said:

If we want to include the underlying type of the typedef here, we'll need a new kind of hashing to capture only the value of the typedef and not how it was written. I don't think it's worth it; let's just not include the definition of a referenced typedef in the hash for now.

So now it looks like the only path we can forward? Or can we accept the regressions described in the patch?

LGTM, though please wait a day or two for any more comments from @gribozavr2 since he's looked at this more closely than I have.

In D154658#4533476, @MatzeB wrote:

This change results in some of our builds (distributed Thin-LTO in case that matters) to fail with missing symbols. At a first glance this seems to emit VTables in some files where it didn't do this before and then fails to resolve some members of that vtable. I'm in the process of analyzing this further and making a small reproducer.

In D154324#4524235, @v.g.vassilev wrote:

@rsmith, thanks for the suggestions! Could you go over ODRHash::AddTemplateName suggest how to fix it to address https://reviews.llvm.org/D153003 and https://reviews.llvm.org/D41416#4496451?

I've done a pass through this file looking for places where we incorrectly add to the ODR hash a type that was written within some other entity than the one that we're ODR hashing, that could validly be spelled differently in different declarations of that other entity. There are quite a lot of them; please see the comments here for places that need fixing.

In D132779#4523783, @jansvoboda11 wrote:

Hi @rsmith, this commit makes it possible for HeaderInfo::LookupFile() to be called with different RequestingModule within single CompilerInstance. This is problematic, since some modules may see headers other modules can't (due to [no_undeclared_includes]). This can permanently mess up contents of the lookup cache (HeaderSearch::LookupFileCache) that uses only the lookup name as the key.

OK, I see. The problem is that the canonical version of the type can be spelled in different ways in different translation units, due to us treating some expressions as being equivalent despite them not being the same under the ODR. For example, we consider these function template declarations to be redeclarations:

This looks correct to me, but it's still a little subtle. Perhaps it'd be clearer to map the method to an integer (0 for copy assignment, 1 for move assignment, 2 for destructor, 3 for equality comparison), and then order them by that integer? That'd be more obviously a strict weak order.

In D154658#4516554, @rjmccall wrote:

LGTM, except, should we have a way to turn this optimization off specifically?

• Add option to disable vptr-based dynamic_cast optimization.

Seems reasonable to me. Given that this affects all of LLVM I'd like to wait a day or so to see if anyone has concerns.

This branch doesn't look necessary or correct to me. We should never call this function on a lazy pointer in offset mode unless we have an external source that produced the offset, and it's certainly not appropriate to cast the offset to a pointer and return it.

Ping.

In D154658#4490468, @rjmccall wrote:

Oh, this does matter on platforms using pointer authentication, because each vptr field is signed with a constant discriminator derived from the name of the class that introduced it.

• Avoid emitting the type_info when detecting whether it would be null.
• Bring back class type in GetVTablePtr and instead don't use it here.

• Mark gep as inbounds.

In D154658#4481225, @rjmccall wrote:

In D154658#4479213, @rsmith wrote:

I think (hope?) we should be able to apply this to a much larger set of cases. Would it be correct to do this optimization unless the vtable might be emitted with vague linkage and non-default visibility (that is, unless we're in the odd case where people expect non-default visibility classes to be the same type across DSOs)? Or are there cases where we might be using a vtable that (eg) doesn't even have the right symbol?

I don't know of any problems other than the total failure of vague linkage across DSO boundaries, so if we just treat that as an implicit exception to the vtable uniqueness guarantee in the ABI, I think you've got the condition exactly right: we could do this for any class where either the v-table doesn't have vague linkage or the class's formal visibility is not default. Our experience at Apple with enforcing type visibility is that it's usually good for one or two bug reports a year, but it's pretty easy to explain to users what they did wrong, and Clang's visibility attributes are pretty simple to use. (type_visibility helps a lot with managing tradeoffs.)

• Address review comments.

In D154658#4479170, @rjmccall wrote:

I don't think it's an intended guarantee of the Itanium ABI that the v-table will be unique, and v-tables are frequently not unique in the presence of shared libraries.

Making the return ESR_Failed; unconditional looks to be the correct change here. We can't continue evaluation past that point because we don't know what would be executed next. Unconditionally returning ESR_Failed in that situation is what the other similar paths through EvaluateStmt do.

Wow, what an amazing bug :)

LGTM, thank you for doing this!

In D153003#4458323, @ChuanqiXu wrote:

In D153003#4456595, @rsmith wrote:

How to we even get into the ODR hasher here? I thought we only applied it to function and class definitions (to which the ODR does apply).

I think this comes from we add ODRHash for RecordDecl.

I think the behavior change for the testcase here is correct, though I'm not sure that the patch is getting that behaviour change in the right way. Per [temp.type]/1.4 (http://eel.is/c++draft/temp.type#1.4),

A constant expression (including the immediate invocations generated for consteval functions) is a full-expression, so destructors should be run at the end of evaluating it, not as part of the enclosing expression. That's presumably why the code is calling MaybeCreateExprWithCleanups -- to associate cleanups for the immediate invocation with that ConstantExpr rather than with the outer context. I think the problem is that we don't have an ExpressionEvaluationContext wrapping the immediate invocation, so we don't save and restore the enclosing cleanup state -- and we can't really wrap it, because we don't find out that we're building an immediate invocation until after we've already done so. Probably the best way to handle that is to create the inner ExprWithCleanups for the constant expression, but leave the cleanups flags alone so that we also create an outer ExprWithCleanups. That'll mean we sometimes create an ExprWithCleanups that doesn't actually run any cleanups, but that's OK, just redundant. One thing we will need to be careful about is assigning any block / compound literal cleanups to the right ExprWithCleanups node. We can figure out where to put them by doing a traversal of the ConstantExpr's subexpression to see if it contains the BlockDecl / CompoundLiteralExpr being referenced by the cleanup.

In D142388#4414375, @aaron.ballman wrote:

In D142388#4412521, @rsmith wrote:

I have a concern with the name of this builtin. People are going to assume it produces a nondeterministic value, and use it for seeding random number generators and similar, and will be surprised when the value produced is actually deterministic, and, worse, might leak information on the stack that was left behind by some previous function call. Can we give this a name that doesn't suggest that it produces its value nondeterministically?

That's a fair point; do you have a suggestion for a name? (I had suggested __builtin_unspecified_value() but perhaps that is too focused on the term of art.)

I have a concern with the name of this builtin. People are going to assume it produces a nondeterministic value, and use it for seeding random number generators and similar, and will be surprised when the value produced is actually deterministic, and, worse, might leak information on the stack that was left behind by some previous function call. Can we give this a name that doesn't suggest that it produces its value nondeterministically?

In D148700#4401353, @jyknight wrote:

Yes, standard attributes aren't supposed to be used for things which affect the type system (although, we certainly have many, already, which do, since we expose most GCC-syntax attributes also as C++-standard attribute syntax!)

Taking a step back for a moment: what is the intended use case for this? My concern is that most of the time you're going to want to make O(N) such queries into a pack of N elements, resulting in O(N^2) compile time, much like we regrettably get from uses of __type_pack_element. If we can avoid the regret in this case by providing a different intrinsic, perhaps we should do so. (For example, we could take two packs of types, and produce a sequence of integers giving the indexes of each of the first types in the second list, in O(N) time. And similarly we could add a __type_pack_elements that takes a pack of types and a pack of indexes and performs N indexing operations at once, in O(N) time, rather than having the program do it in O(N^2) time.)

LGTM too.

Sorry for the late comments.

In D150875#4353384, @erichkeane wrote:

We are the only of the major compilers with this "extension" (I hesitate to call it that, as I'm not sure this FITS in the 'extension's permitted by standard)

LGTM with the extra test

I think this would be an interesting test:

There's some corresponding code in lib/CodeGen that can be deleted too:

This looks like a nice improvement to me.

I wonder if there are any cases where the "deduction guide" is so broken that we shouldn't be creating a CXXDeductionGuideDecl at all. For example, if there's no trailing return type, then the declaration is syntactically invalid (and looks more like a constructor than a deduction guide), and it's not clear to me whether creating a CXXDeductionGuideDecl really makes sense.

LGTM, I'll leave approval to @shafik :)

I think that the bugs that this was aiming to fix were fixed by rG1e43349e321694d7fee3d77cb691887ad67fb5d7, which means that we should not ask whether the constraints of an inherited constructor are satisfied any more. Instead, overload resolution will look at whether the constraints of the original constructors are satisfied. (More broadly, I wonder whether DiagnoseUseOfDecl should be considering constraints at all -- the model in the standard is that any time you name a function, you go through overload resolution, which considers constraints, so DiagnoseUseOfDecl should only ever be duplicating work that's already been done by overload resolution, but it's possible we're not entirely following that model. Still, we should be able to avoid redoing the constraint satisfaction checks in the common case where overload resolution already did them.)

Are there any calls to Sema::ActOnParamDefaultArgumentError left? If not, can you also remove the function?