Sorry for the delay.

lgtm

If we look back at the original intention of our MSVC compatibility work, we tried to accept as much invalid code as necessary to parse "system headers". System headers were widely interpreted to bethe MSVC STL, ATL, and the Windows SDK. So, even if MSVC defaults to delayed template parsing, if system headers parse with /Zc:twoPhase enabled, maybe we could pick a different default. We'd document the behavior change, of course. Delayed template parsing is an ongoing source of bugs (https://llvm.org/pr50676) and maintenance overhead, so it would be worth diverging from MSVC a bit if possible.

The spill is conditional on %al ...

The code change is just adjusting some if conditions and changing indentation, so there's no reason to hold it up with my design questions. Please consider implementing the refactoring suggestion as a follow-up, though.

I think the verifier is doing the right thing, I think it's a bug that @llvm.memcpy is carrying a returned attribute on the first argument. I'm not sure how that is happening, but whatever sets that up seems like a bug.

Thanks for finding this, I think it looks good.

I fixed it and it went green, thanks for the fix and the heads up though:
https://lab.llvm.org/buildbot/#/builders/127/builds/12308

IMO it would be more resiliant to hide the index or the pointer from the compiler, let me try to put together that fix.

That's a good point, if this attribute disables vectorization of integer math, the docs should say as much.

I agree, it's a choice between 2 and 3. 3 would be consistent with what happens when the existing profraw file is from another binary, right? Maybe do that? It risks losing coverage data, and means the corrupt file is persisted. I guess that's best, since it will cause llvm-profdata merge to fail later.

This fixes an urgent crash bug, so let's go ahead and land it.

This matches my understanding of the semantics of noimplicitfloat, but I'd like to get a stamp from one of the other people I added to check my understanding.

Sorry about all the nits, bounds checking is all about attention to detail.

lgtm

I think it's worth the time to carefully refactor this code so we don't end up in a similar situation, maybe in the value profile data.

+1

In D103752#2806569, @eugenis wrote:

The buildbot went green before your revert has landed.
No idea what's going on, but this change does not seem to be the cause of it.

I think we can say that _HAS_EXCEPTIONS=0 is an important configuration. We use it in LLVM, you can grep for it:

$ git grep _HAS_EXCEPTIONS ../llvm
../llvm/cmake/modules/AddLLVM.cmake:      list(APPEND LLVM_COMPILE_DEFINITIONS _HAS_EXCEPTIONS=0)
... others

In D103495#2806097, @jyknight wrote:

Won't this change cause weird effects with LTO, when you're merging multiple TUs' global_ctors arrays before emitting? Won't this end up reversing the order of the files, as well as the order of the functions within a single file? That does not seem likely to be expected, right?

We are observing some crashes in coverage, but we haven't root caused them yet: crbug.com/1216811
So I guess we need to keep waiting for the dust to settle. @aeubanks was able to repro the crash there, but it's not deterministic.

lgtm

I think we do need to think about globalopt. Consider this example:

extern "C" {
int printf(const char *, ...);
extern int sharedState;
int sharedState = 0;
int init1() {
  asm volatile(""); // block globalopt
  return ++sharedState;
}
int init2() { return ++sharedState; }
int gv1 = init1();
int gv2 = init2();
}

lgtm

+ various .ctors stakeholders + @MaskRay + @mstorsjo

Seems reasonable to me.

I see, thanks.

OK, right, the issue we were trying to solve is that LLVM was crashing when byval (+inalloca, etc) types mismatched somehow.

I think MachO doesn't have comdats, so we need to leave the __profd_ linkage as it was. I think private linkage also prevents atomization, which inhibits GC / dead stripping. I don't think this change really makes sense for MachO, so you could reasonably reland with that change.

I don't like the idea of having the IRBuilder look at the call target and implicitly setting attributes, but I am open to revisiting the backend change. Here's a rationale for going back to the old behavior:

• calling a function with the wrong function prototype is UB
• calling a function with mismatched ABI attributes is very much like prototype mismatch, so it's UB
• LLVM can generate whatever code it wants in cases of UB. So, if we have a direct callee, why not have the backend implicitly use the attributes from the callee?

lgtm

There's some horrifying index manipulation involving unsigned overflow that I had to avoid by checking if either AttributeList is empty.

This reduced object file size of base_unittests.exe, compiled with coverage, optimizations, and gmlt debug info by 10%:

#BEFORE

lgtm

In D103495#2794870, @wolfgangp wrote:

For PS4, we use the .ctors scheme, and so the initialization order was suddenly reversed, which was not noticed for a while until the user had a dependency on the previous initialization. And using init_array or not has currently no effect on the order of emission of global_ctors.

Thanks! This is really cool. It will probably help reduce coverage object file size by a lot. Let me see if I can get some numbers for that.

lgtm

You're right, I don't see a way to directly merge two AttributeList objects. I looked at AttrBuilder::merge, but AttrBuilder makes AttributeSet objects. I guess it makes sense to have an AttributeList method to do this.

In D103495#2794667, @ychen wrote:

Is there anything preventing us from using the existing priority field to define the order instead of introducing the order among initializers with the same priority? If we go with 1., there would be no way to say that "the order does not matter" right?

I'd suggest something like:

CI->setAttributes(CI->getCalledFunction()->getAttributes().removeAttributes(Ctx, llvm::AttributeList::FunctionIndex)));

lgtm, thanks

I think this would be an unfortunate code size and startup time regression for Itanium C++ inline variables. The existing code was written under the assumption that vague linkage (GVA_DiscardableODR) implies no initialization ordering, but I think you've found a valid counterexample.

The main impact of this change seems to be that PGO data for non-comdat code is now private instead of internal. That's a good change: PGO data has really high object file size overhead, and LLD already discards PGO symbol names from the PDB because they are so large.

• do the feature check

lgtm with the comment update

lgtm

Ew. I guess I was wrong, the getCalledFunction version was better. Sorry about that. :)

lgtm

I don't have plans to work on this, but it's a pretty straightforward starter project if anyone is interested.

In D92515#2774647, @mstorsjo wrote:

Would it be a good idea to bump the default MSVC version that Clang presents itself as to 19.14 at this point, to remedy this situation?

We are moving to make ghash the default, so I don't think we need this. I have a follow-up to pass -gcodeview-ghash: D102888 I think we can close this as obsolete.

I suppose I should've updated D43881, but I made a new one at https://reviews.llvm.org/D103287. Oh well.

In the absence of review from ARM, I think we should land this as is.

The test failures are from the libfuzzer tests, which suggests that the coverage instrumentation pass has the same problem that msan had. I see an instance of the ZExt attribute there, that doesn't match the call site:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp#L428

lgtm

I think this look good. Adrian, are your concerns addressed?

lgtm

Nice, this seems to be working out.

lgtm

Classic.

Yep, this is more or less exactly what I had in mind. :)

If ExitProcess still unloads DLLs before exiting, I believe there are ways to call TerminateProcess to avoid this.

I'm in favor of strengthening _exit to ExitProcess. Fewer configs is better. Users won't have to discover these flaky shutdown crashes and add this customization.

lgtm, thanks.

In D102888#2773545, @aganea wrote:

@rnk as a next step, you would probably want to re-do D43881 so that -gcodeview-ghash is enabled by default when building LLVM?

I see test failures on Windows that appear to be related to CRLF handling. I think the CHECK-NEXT checks aren't working because something is doubling up the newlines in the test output. That seems unrelated, so I suggest dropping the -NEXTs as necessary to get it past the presubmit testing.

• use /debug:noghash

In D102885#2772493, @aganea wrote:

LTGM.

If you're making /DEBUG:GHASH the default, can we switch back to the "classic" merging?