Should these be unnamed labels so they don't appear in the COFF file symbol table? I'm guessing ml64 doesn't produce symbols for these labels, but correct me if I'm wrong.

Test cases look good, thanks!

Thanks for expanding our docs!

+1 for the exception code. I think it may appear as the process return code and some systems will log that, but logging it ourselves, too, is for the best.

In D126977#3590611, @arphaman wrote:

@rnk this is option is needed to vendor the Apple clang toolchain in Xcode. Unfortunately we don't have any references to it in the upstream llvm-project repo, but this is breaking our downstream integration. Can this be reverted?

lgtm

Thanks, sorry for the delay.

I think Richard had some concerns in the other review that this may not be enough to really guarantee initialization order within the TU. I couldn't say either way, I shouldn't review this code. Conceptually, this change seems small enough to me. Can we ask @aaron.ballman to take a look?

Seems useful

+@nikic

I don't have a great answer here, but yes, dllexport macro norms sort of run directly counter to the normal ways that people use PCH. It seems like projects will need to have a library module / PCH file for building a DLL and then a PCH / module for consuming the DLL.

Thanks, this is a good idea.

+@luken-google

lgtm

Does the MLIR LLVM dialect not have comdats? That seems like a missing feature, a lack of fidelity that would prevent lossless conversion from LLVM IR to MLIR & back. Clang uses comdat groups for some esoteric features, the D5 destructor alias group, and MSVC RTTI.

lgtm

@labath, ping, I see you just got back from vacation and this is a lot of code, but any high level design feedback would be helpful to know if this is the right direction.

lgtm, thanks

lgtm

lgtm

Well, I guess we're out of luck, but that seems like a very poorly considered requirement from the standard. If we can't use comdats for inline variables, every time you include a header with a dynamically initialized variable, it will generate extra initialization code in every TU that cannot be optimized away. This reminds me of the problems people used to have where every TU including <iostream> emitted extra initialization code.

lgtm, sorry for the delay, do you need someone to push this?

In D125723#3553664, @aaron.ballman wrote:

In D125723#3550956, @steplong wrote:

Appreciate the help! It's not clear to me how to go from the strings "Os", "foo1", "foo2", "foo3" to adding "-Os -ffoo1 -ffoo2 -ffoo3" to the compilation line for that function

I may be misunderstanding the issue here, but what I had envisioned was that the OptimizeAttr would take either a string or integer argument (I don't know how easy that will be to model in Attr.td, FWIW; it may require adding a new kind of Argument for attributes) and then add a "fake" Argument or use the AdditionalMembers field to add a member that stores the converted "value" of that string or integer in whatever form makes the most sense for the semantics (I was envisioning an enumeration for the various kinds of optimization options, but maybe that doesn't work for the -f arguments?). Then, when doing CodeGen, you can look at the function to see if it has an OptimizeAttr, and if it does, use the fake/additional member to determine what information to lower to IR (or not lower, as the case may be). Does that help get you unstuck somewhat?

Btw, one possibility would be to not support any -f flags initially and only support optimization levels, if that's easier. We can add support for individual flags at a later step but still get utility out of the attribute this way.

In D126581#3544738, @thakis wrote:

(I think chances are low that someone changing the .py script to fix the GN build would break the bazel build or the other way round: The script hasn't meaningfully changed in well over 2 years.)

(If we end up doing a bunch of changes to the .py script later on for some reason or another, it might make sense to have a gn and a bazel version of that script at that point, though.)

I think it's fine to implement this, but I wanted to share some of the motivation for the current state of things. In our experience, the majority of uses of pragma optimize were to work around MSVC compiler bugs. So, instead of honoring them, we felt it was best to ignore them with a warning (-Wignored-pragma-optimize). Of course, that warning is typically disabled in build systems of large projects, so our current behavior can still surprise users. Implementing the feature is probably the most predictable and least surprising thing Clang can do.

lgtm

lgtm

lgtm

I'm somewhat supportive of the goal here, but I think there are still some underlying issues.

I am reminded of the perennial problem of "optional" protobuf fields that, when omitted, will cause production crashes.

lgtm

lgtm

@MaskRay, can you review this? I think you have been thinking the most about how to parallelize linkers, and what fundamental data structures are most important.

This seems reasonable to me, but surely this is not the complete list of places where we must use the VFS?

lgtm, thanks!

I agree with David, I would like to see LLVM move in the direction of never emitting empty functions. These are just labels that snap to the next function in the same section, and that's silly. I'm not sure what happens if you use function sections. We should just emit some trap instruction, and let the linker do identical code folding (ICF) to merge them back together. This will regress code size, but I doubt out users will complain, and ICF will recover most of the size regression.

I was unable to find any documentation for the meaning of AArch64 addrspace(256), and I wasn't able to figure it out after studying the code in llvm/lib/Target/AArch64 for ten minutes or so. The change seems fine, but please add some documentation as a follow-up. X86 has some of its address spaces documented here, not that this is the best place:
https://llvm.org/docs/CodeGenerator.html#x86-address-spaces-supported

Nice!

I haven't given any input yet because I haven't yet seen a reduced example of the conforming code that is broken by this change. If someone can put together a small godbolt example that shows the issue affecting ADL, I'd appreciate it.

Looks good to me. I generally agree with Adrian's suggestions.

Sorry, I can't easily test this patch, but I do think it's probably the right direction.

In D123534#3507891, @hctim wrote:

Not sure about PDB. I did run a quick test with gdb, and very unscientifically, didn't notice any difference in usability or errors between pre- and post-this patch on a clang invocation under gdb.

I wanted to give some early feedback that this broke our internal build of CPython, and we put together a local workaround, so we aren't blocked. I don't fully understand what's going on or have time to follow up, but I wanted to give a heads up that this seems like it could be a disruptive change for users.

lgtm

lgtm

This seems reasonable, but I worry about the consequences of creating lots of unnamed global variables. What will gdb do with so many unnamed globals? What will the PDB linker do with all these unnamed globals? I can't answer these questions, and you're welcome to try and find out, but I predict there will be problems, so just be aware of that possibility.

In D124666#3490871, @frederic-tingaud-sonarsource wrote:

By the way, in the current state of the code, there is no risk of such conflict between typo correction and UnqualifiedBase, because when an unqualified base exists the lookup result will not be empty.
That's why the test was not failing even without moving the compatibility first.

lgtm, thanks!

In D124762#3485478, @sgraenitz wrote:

I guess testing must be split in two:

• Clang wants to make sure the "funclet" bundle operand gets emitted for ObjC ARC runtime calls on Windows. Maybe that fits into clang/test/CodeGenObjC/gnu-exceptions.m?

Which pass inserts the calls to these intrinsics? Can that pass be responsible for calculating funclet colors and adding the operand bundles to the intrinsics? That would match what we do for other instrumentation passes (ASan, PGO). The pre isel lowering pass can simply carry over the funclet operand bundle if present. I think there is a utility for carrying over that and similar operand bundles.

I plugged this into godbolt to confirm the behavior of MSVC:
https://gcc.godbolt.org/z/v3P3WbxG3

Looks good to me, just one style comment.

lgtm

lgtm

lgtm, thanks!

In D123932#3457579, @arsenm wrote:

I tried this and it doesn't quite work. Maintaining the current behavior requires 2 fields; that split-stacks requested and whether or not it was actually emitted. Do you think it would be better to just move the StackSize == 0 && !MFI.hasTailCall() check into a helper in MFI, track a second field in MFI, or just keep having the AsmPrinter look at the attribute?

In D122963#3456350, @LuoYuanke wrote:

I think we all agree that this is a bug. Given there is no better solution for it, I'd like check in this patch first. We can revert the patch when there is better solution proposed.

I read through the situation described in PR37807 the two relevant commits (rG9cc1ffadc5ad06ab846a7da95a1afb874b9f3d98 and D48444), and I think I understand the two issues.

This is sort of reasonable, but I feel like people might disagree so I won't go ahead and stamp it.

lgtm

lgtm

In D123872#3454636, @jyknight wrote:

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...

It might be possible to recompute these, but I don't think it would be reliable.

Thanks for running the experiments, I think I'm convinced we need to do this. @hans, can you review the implementation? I removed Adrian and Rui, who are no longer regular contributors.

Generally speaking, this sounds like a good idea to me. One time in 2019 I used -ftime-trace+ClangBuildAnalyzer and it told me that std::unique_ptr was the most expensive template because it is instantiated so much. Those results don't even capture the -O0 object file size impact.

Looks good to me after the fact.

How old is 19.20? Do we need a version compatibility check here to ensure that we don't break users of MSVC pre-19.20? I believe that is spelled LangOpts.isCompatibleWithMSVC(...).