As somewhat coincidentally just discussed in the thread on https://reviews.llvm.org/D124728 the target hooks don't really make sense as implemented here.

In D126324#3536785, @aaron.ballman wrote:

The changes so far look sensible, but I think we should add some more tests for a few situations. 1) Using a const weak symbol as a valid initializer should be diagnosed (with a warning? with an error?) so users are alerted to the behavioral quirks.

Good improvement, but an additional change to wording for just the zero-arg non-prototype function declaration case could help a lot. The fact that zero-arg it's only being warned about because of the "...because of" note isn't particularly clear -- especially when the "because of" isn't emitted.

In D124728#3534101, @tkf wrote:

Can this patch land? Is there anything else that needs to be done?

FYI, clang/www/cxx_dr_status.html is not intended to be edited manually; it's built from automation based on test cases added to clang/test/CXX/drs/, which this change didn't add.

In D126079#3528275, @tianshilei1992 wrote:

I did have one patch locally that only keeps the function shouldExpandAtomicInstInIR(Instruction *) and in the body it has something like if (auto *SI = dyn_cast<StoreInst>(I)). In terms of readability, IMO it is same as overload. However, of course it can reduce number of APIs. I can use that method, but it looks like the controversy here is readability.

IMO, this change hurts the readability of the code, not improves it.

Thanks!

In D125773#3523459, @rsmith wrote:

Header modules are part of the C++20 standard (where they are called "header units"), and module maps are an intended way for Clang to provide this functionality in C++20 mode. I don't think turning this off by default in C++20 is the right forward-looking plan; rather, I think we should be thinking about moving towards header modules simply always being something that Clang can do, with no flag to control that.

LGTM, modulo final comments.

This confuses me.

I find the option names you have a bit confusing. I'd like to suggest calling them, instead:

The warnings for this case aren't great:

int foo();

Couple more comments, but looks pretty good.

In D124895#3492632, @keith wrote:

In D124895#3491724, @jyknight wrote:

Also, while your goal of improving the heuristics seems reasonable, perhaps we ought to have a --archive-format option for objcopy, so that users can be explicit if they desire. (We already have --format for ar, and for the cross-compiling case it's basically required -- there's simply no other way to choose the correct behavior for archives with no files, like llvm-ar rcs x.a).

From a UX perspective I hope we can come to a reasonable patch here instead so folks don't have to know about these details in this case. In the case of no files I don't think the decision really matters for this use case at least, although I get that issue in general.

I don't think we ought to modify the archive reader like you've done, because we don't need to tell the difference between BSD/DARWIN when reading, only while writing. Thus, going through the trouble of distinguishing them in the reader is wasted work. Instead, please add a separate function, which ar and objcopy can call before constructing an ArchiveWriter, which returns the more "refined" Kind.

Isn't libxar still required by the lld.ld64 -bitcode_bundle option, so doesn't LLVM need to continue to use it despite it being deprecated?

The test case added here appears to sometimes fail due to output ordering differences. E.g. https://lab.llvm.org/buildbot/#/builders/16/builds/28225

This change has caused our internal PGO+NewPM builds of clang to have become non-reproducible (that is: the binary is different every build, when built with a Clang that includes this change). The problem appears only when providing profile data. I am not sure why this change causes a problem -- perhaps some of the analysis data ought to have been invalidated by some other pass, but wasn't?

-fembed-bitcode was originally added by Apple to enable them to recompile
3rd-party-developers' software with a newer compiler, or for new CPU
targets, without the source code. It uses the normal non-LTO optimization
pipeline, and emits bitcode corresponding to the binary object also
emitted.

Looks good to me, whichever way you go regarding rsmith's comments.

Sidenote: Oh, yikes! I didn't even notice before that this code is doing reinterpret_cast from a GHashCell* to a std::atomic<GHashCell>*. That's...kinda not great...

In D122983#3454406, @aaron.ballman wrote:

In D122983#3452994, @rsmith wrote:

I think we should just make this an error by default in C99 onwards;

Out of curiosity -- do you think we should remove the DefaultIgnore in C89 mode so that we warn by default there (perhaps as a follow up)? Also, I presume you expect diag::ext_implicit_lib_function_decl to behave the same way (warn in C89, warn-as-err in C99 and up) as part of this patch?

In D123642#3450110, @efriedma wrote:

I disagree with this on principle -- IMO, it is basically a historical bug in GCC that it ignores the type alignment, and we should NOT try to match that -- it's dangerous.

gcc has always behaved this way, and I don't see any indication they intend to change it. I mean, you can call it a bug, but at the end of the day the bug reports will land in our bugtracker, not gcc's.

I disagree with this on principle -- IMO, it is basically a historical bug in GCC that it ignores the type alignment, and we should NOT try to match that -- it's dangerous.

Looks reasonable overall.

What's the status of this? Did the GCC proposal go anywhere? I'd be happy to see this move forward if you're also pushing it on the GCC side.

LG, just some minor comments.

Sometimes ignoring the attribute and performing zeroext ourselves is a
workaround for ABI mismatch.

Starting by looking at the test cases, I've got some suggestions on making the diagnostics a bit less confusing.

This discrepancy in behavior seems to have arisen because getFnAttr was only added recently in 7557d6c896d3418216e82c0d0cf3b0708f2145bb by @aeubanks; I suspect it was an oversight not to have it match the longstanding behavior of hasFnAttr.

In D120159#3415112, @kda wrote:

This appears to have broken buildbot: sanitizer-x86_64-linux-fast

Validating if reversion clears it up.

Then will revert.

Given that this is an error error: functions cannot be declared in an anonymous struct (thus you can assume that function declarations inside a struct imply that the struct MUST have a linkage name)

struct {
  static int foo();
};

then I don't think there's any conceptual reason why we cannot emit the desired diagnostics without the undesired side-effect.

Ping; any more comments?

Expanded the comment slightly in 4a6fab793fa00b8155c2c08a6fbad7ee3e7feba6.

Being consistent with allocalign's proposed semantics sounds good to me.

In D118511#3371432, @tstellar wrote:

I'm fine with reverting if you think this is the best solution. I just would like to conclude soon so I can make the final release candidate.

In D120634#3352084, @ldionne wrote:

Does this mean we'll only implement source_location with Clang? Or does GCC provide a similar builtin? If so, are both compatible?

Update for review comments.

So, in doing further testing, I've noticed a problem. Currently, clang cannot compile code that calls a consteval function in a default argument. (see https://github.com/llvm/llvm-project/issues/48230 and https://reviews.llvm.org/D119646)

In D120634#3347963, @philnik wrote:

Looks like you have to run ninja libcxx-generate-files.

Update per review comments.

Fix and test __impl lookup within the definition of std::source_location.

In D119051#3316026, @dblaikie wrote:

Ah, looks like this is the existing https://github.com/itanium-cxx-abi/cxx-abi/issues/66

In D120159#3349069, @aaron.ballman wrote:

Ah, okay, that's a good point. I was thinking this would show up in the AST in places like:

template <typename Ty>
auto func() -> Ty;

int main() {
  func<decltype(__builtin_source_location())>();
}

when we go to print the Ty type from the template. Does that not happen?

In D120159#3341224, @aaron.ballman wrote:

Btw, I think there may be some functionality missing for AST dumping, so I'd like to see some additional tests for that.

Update per review comments.

Minor tweaks to comments and docs.

This can be submitted first, separately from the rest of the stack.

It's not UB to provide invalid alignments to aligned_alloc, but that's not something the langref covers.

Agree, we do want this. The general strategy (which I don't think should be changed) is for LLVM to be aware of standard libc functions, and infer attributes for them. (We do want to get it out of the business of being aware of the C++ stdlib functions, however.)

I'm happy with the LangRef semantics and wording that here now.

In D115955#3308207, @ABataev wrote:

It was reverted in 14, currently only in trunk.

In D118804#3302675, @rsmith wrote:

I support this revert.

In D118804#3301746, @Lapenkov wrote:

Is it hard to retrofit this diff into LLVM 13?

Rebase, and preserve (and modify) test-case.