Let's close it.

Closing, we left the test alone, it still uses -fuse-ld=lld-link2. Perhaps in the future we should reconsider this, but that's how things stand now, and we aren't going to land this patch as is. + @MaskRay , who has thought about -fuse-ld= semantics.

+@cjdb , do you have time to review this?

lgtm

lgtm

One of our use cases for having a "large" attribute for global data is to have a way to mark instrumentation data as large. We would use this for ASan global metadata and PGO name data, which are not accessed frequently during program execution, but contribute significantly to binary data size.

FWIW after speaking with @thakis offline, I do think we did "everything right", in the following sense. We added the deprecated attribute to the class and left it there across one full release (roughly 1 year). This should leave enough time for people to notice the warning when updating libc++, turn it off temporarily with -Wno-deprecated as they fix their code, and then re-enable the warning. I think this is the right model we want to give folks.

Following up, the vararg fix is here: https://github.com/llvm/llvm-project/pull/65692

The reproducer in the commit message no longer asserts for me, maybe we can close this.

lgtm, thanks!

In D154294#4635441, @efriedma wrote:

The CFG becomes more complicated in the sense that there are more edges... but it becomes simpler in the sense that code which generically supports EH pads should just work without additional SEH-specific knowledge. The latter property seems more valuable because most people working on the backend know nothing about SEH.

Thanks for the patch, this is gnarly code. At the time I was probably thinking that flat vectors are good for performance, and that we'll have tons of line table entries, but as the inline location handling evolved, it's gotten quite complicated.

lgtm

In D154294#4630056, @pengfei wrote:

bb.5.invoke.cont4 to landing pad (I assume it's bb.7.ehcleanup) doesn't matter to me here. I analyzed the problem was the edge from bb.3.invoke.cont2 to bb.4.__except was removed in BranchFolding. This is the direct problem I wanted to solve here. It seems we are talking about independent problems. I didn't research the other but I think it doesn't solve my problem.

I think that's reasonable. libgcc probably retains them out of some desire for longer term backward compatibility, and I think we're a little less constrained there.

+1, this use case has long been addressed by the assertion mode, and then the new safe and hardened modes.

I haven't heard any serious concerns, and I hope I addressed your questions. I plan to go ahead and land this, but feel free to modify or revert if you have concerns.

In D154294#4629535, @efriedma wrote:

hasEHPadSuccessor() is actually false for the block that throws the exception. I think that's a bug... but everybody else who's looked at this code apparently disagrees with me.

I wanted to follow up on this, it was hiding at the bottom of my inbox.

In D153690#4627938, @hazohelet wrote:

@rnk This reproducer code should be accepted for clang to be aligned with msvc's behavior, correct?

At various points I have tried to untangle the meaning, purpose, and function of these various stack adjustment routines, and I never felt like I had a satisfactory understanding. I trust that you got to the bottom of this.

This seems like it will generate warning cleanup work for vendors, so I'll mention #clang-vendors , but I think the code looks good. I don't think it makes sense to add a fine-grained diagnostic category to allow people to clean up incrementally in this case.

I think this kind of error ("relocation against symbol in discarded section") is really hard to debug, and probably deserves a whole page of documentation. I think the best option would be to emit a short link to online documentation describing the issue category in detail, something like https://lld.llvm.org/missingkeyfunction.html .

In D157136#4583632, @aganea wrote:

I'm also wondering if instead of this patch, we couldn't craft something simpler, an heuristic that "fixes" the section header and the section symbol. Usually they seems to come in a pair, .text$mn, .text$x or a trio if debug symbols are involved: .text$mn, .debug$S, .text$x. Opinions?

I put together a fix here: https://github.com/llvm/llvm-project/compare/main...rnk:llvm-project:fix-vararg-align

+@rupprecht @aeubanks, can you review this Bazel change?

I need to get to it, my recollection is that @mstorsjo ran into the same issue here for mingw and made some changes, I wanted to go dig those up as a starting point. I may have completely forgotten things though.

lgtm

lgtm

+@efriedma, since I think we had that ongoing thread about what to do with i128 alignment in data layout. Is this a reasonable way to handle the datalayout migration?

Regarding large code model jump tables, I think I filed an issue for alternative, relocation-less jump table encodings: https://github.com/llvm/llvm-project/issues/62894

lgtm with some tweaks to the test.

It took me a while to find this code to understand the issue. The problem is that there is an inconsistency between the GVA linkage and the LLVM IR linkage for these constructors. In this godbolt example, ??0?$TInputDataVertexModel.. is marked internal and marked comdat, and that is uncommon. PGO instrumentation appears to do the wrong thing in that case. Is that more or less what's happening?

Thanks, I think this is a clang bug. As I understand MSVC's behavior, we should not pass highly aligned variadic arguments indirectly: https://gcc.godbolt.org/z/Kr67xWTeE

I went ahead and pushed a revert of this, since this change was pretty disruptive, at least in our experience. I could be convinced that it's worth sunsetting this extension for accepting C-style format string macro code, but that doesn't seem like the original intent of this change. Maybe that's the right thing to do, but it should be an intentionally considered change.

lgtm, if @asl is offline, and it's useful

I'm not that familiar with the typical compatibility requirements for DLLs in the Windows system folder, but it seems pretty disruptive to ship a DLL in the system folder, and then ship ABI incompatible changes without increasing the version number. Wouldn't this all work if the STL increased the version number of msvcp140.dll instead?

lgtm, thanks!

In D61670#4604486, @mstorsjo wrote:

In D61670#4604145, @rnk wrote:

cc +@aeubanks @jyknight to consider using the code model for this purpose

Hmm, I don't quite understand this comment; do you suggest that we after all should use the code model for controlling the use of .refptr stubs too - didn't we conclude earlier that it isn't a great fit for that?

cc +@aeubanks @jyknight to consider using the code model for this purpose

lgtm

lgtm

• rebase

Ping, any outstanding concerns?

Thanks for the review, hopefully all comments are addressed.

• apply fixes

Also assuming that /Ze is similar to our -fms-extensions which I believe is the case, but not completely sure

So, I'm strongly in favor of the feature, but I want to get feedback on the name.

Seeking a code reviewer, +@aeubanks, @vitalybuka

I don't think this is the right thing to do, I think I recall saying as much on some other review. The MSVC docs say that /Za//Ze control _MSC_EXTENSION, and as I understand it, /Za is more like our -fms-compatibility flag, so it makes sense to control this macro with -fms-compatibility.

lgtm, thanks!

That doesn't handle the second of your test cases though, where dllimport is put on the member directly:
...
It's not clear to me how we'd want to handle that. I don't think it comes up in libc++, and I can't think of any code that would want to do that either, really.

So, as I understand the discussion, after this patch, it will still be possible to get dwarf and codeview in the same compile, but users will have to resort to cc1 flags.

I think MSVC defines _MSC_EXTENSIONS under one of its compatibility modes as well, so it's not a good indicator of "is this compiler MSVC-like". I think we should be exposing the __cpudex builtin any time we are being MSVC-like, not under fms-compatibility. Would that make things sensible again?

What are the next steps here? Have concerns been addressed? The CI failures seem unrelated (libcxx tests is an AIX bot out of disk, clang CI is an interpreter test that seems unrelated, but please confirm).

Thanks! My concerns are addressed, but please confirm that Eli's are too.

Any thoughts on this? As I understand it, this is the last remaining strict weak ordering violation in llvm-project, and after that folks can enable the checks for this in libc++. The other two changes (D155811, D155809) landed.

This looks correct to me.

You can see where the eh cont table was added here: https://reviews.llvm.org/D99078

Here are some comments, I have to run, so it's not complete, but hopefully still useful.

lgtm

lgtm

Thanks!

I agree that we can't emit all labels with the .L prefix into the object file symbol table. That would be prohibitively expensive.

This approach makes sense to me.

Thanks!

In D156488#4540742, @nemanjai wrote:

Thank you for the patch. This is actually interesting and is a bit of a change to how we thought the code was do be structured. For us the affinity between CodeGen and MCTargetDesc was an indication that the libraries, although separate, are dependent on each other. Of course, this is not necessary so I am in favour of this refactoring.

I don't want to approve this patch immediately, not because I am opposed to it, but because I think it should be seen and reviewed by all interested developers for the PowerPC back end so that we can change the mindset about these libraries.

• rebase
• rename call lowering file