In D94355#2500795, @joerg wrote:

Some targets already do this. Please check that you don't create regressions, especially on PowerPC.

In D82862#2498785, @hans wrote:

The motivation for my change was really just to make ThinLTO compiles work the same as non-ThinLTO ones.

Maybe the fact that -x86-asm-syntax=intel doesn't affect inline asm is a bug. I wasn't aware that Clang and GCC's -masm= flags behaved differently in that way, but that certainly suggests there's a problem here.

So I'm wondering, if I remove the above setAssemblerDialect line and revert this commit, we should have inline asm (both module-level and GNU function-leve) respect the target-selected asm dialect variant both for ThinLTO and non-ThinLTO, so they should match again. Would that also solve the problem you were originally tracking?

In D82862#2498242, @uweigand wrote:

Hi @hans , we're having some issues with using the AssemblerDialect mechanism to support both the GNU assembler and the IBM HLASM assembler in the SystemZ back-end. See also the discussion started here: https://lists.llvm.org/pipermail/llvm-dev/2020-November/146875.html

One of the issues that showed up is what you're refering to above:

That flag however should not affect the *parsing* of inline assembly in the program.

I'm wondering why this is true. I mean, it is true that the flag currently does not affect parsing of inline asm, but I'm wondering whether it *should* be that way.

Note that the -x86-asm-syntax=intel LLVM flag is used to implement the -masm=intel clang command line option, which exists also in GCC and where hopefully the two compilers should be compatible. But in GCC, that flag is documented to affect parsing of inline assembly just like it affects output. See https://gcc.gnu.org/onlinedocs/gcc-10.2.0/gcc/x86-Options.html#x86-Options

-masm=dialect

Output assembly instructions using selected dialect. Also affects which dialect is used for basic asm (see Basic Asm) and extended asm (see Extended Asm). Supported choices (in dialect order) are ‘att’ or ‘intel’. The default is ‘att’. Darwin does not support ‘intel’.

What is the reason for treating this differently in LLVM?

Based on @hans 's feedback, I added a new enumeration kind called RelOffsetArrayKind, and remove IsRelative flag.
This separates the lookup table and relative lookup table IR generation.
Do you think this helps for the abstraction?
I feel like if it might be better if we can generate the relative lookup table in the first place instead of generating it in a separate pass.

Great stuff, thanks for working on this!

Looks great, thanks!

lgtm

In D92335#2426457, @tejohnson wrote:

lgtm. Thanks for fixing this, and that's a nice cleanup on the existing handling! One request - could you submit the change to the current handling and associated test changes first, with the symver handling/test as a follow on?

Moving the "Append the module inline asm string" code down, and simplifying it in the process.

Adding test case where the symver doesn't get imported.

lgtm

I just realized this makes Linker/ depend on Object/
I'm not sure whether that's a problem or not, but we could avoid that by storing the symvers in a metadata node again.

Pulling in the symver directives in IRLinker::run() instead. (I didn't do a separate function since it's just a few lines, but happy to do that if you think it would be better.)

In D92335#2424693, @tejohnson wrote:

Thinking through this some more, I think I was making it too hard. I think we should just be able to collect the symvers out of the inline asm when we do the IR linking. However, not in the place you've modified below, as I note there. Instead, take a look at IRLinker::run(), where it appends the module inline asm strings together if !IsPerformingImport. I think here you could add a case where IsPerformingImport, and just invoke ModuleSymbolTable::CollectAsmSymvers here and directly add any symver you find to the DstM (probably do all this in a helper function). I was thinking that we might need to carry it via module metadata to avoid parsing it during IR linking, but I'm now not sure that's necessary - I don't think it should add significant overhead. Let me know if you run into issues with that approach.

In D85808#2424908, @jansvoboda11 wrote:

I'm seeing build failures that might be triggered by this patch: http://lab.llvm.org:8011/#/builders/109/builds/3714

Seems like a good idea to me.

Seems reasonable to me, but someone who actually knows clang-scan-deps should take a look too.

Thanks for the pointers!

(I added a naming comment, but feel free to resolve that as you feel is best.)

lgtm, probably check with maskray that he's happy first though.

In D87448#2398082, @mstorsjo wrote:

In D87448#2397386, @hans wrote:

In D87448#2397025, @mstorsjo wrote:

@hans - Can you check whether this works fine with libfuzzer and LLD's ICF?

Yes, this appears to fix the problem we saw. (I tried on Chromium's base_unittests.exe, where ICF time dropped from 13 s with the bad patch to 73 ms with this patch.) Thanks!

Awesome, thanks! Do you want to have a look at the patch itself and review it, or does @rnk have time? (It's his original suggestion, to emit xdata in .seh_endproc, but the implementation is entirely trivial.)

(I also have a couple more patches that can go on top of this, that simplifies things a bit further, and emits pdata together with xdata at the end of the function.)

In D87448#2397025, @mstorsjo wrote:

@hans - Can you check whether this works fine with libfuzzer and LLD's ICF?

For Chromium, the latest patch seems to fix the issues we saw. Thanks!

In D91083#2395780, @vtjnash wrote:

Thanks for identifying that. I think I can re-land just without that part of the test. It is not essential, I added it just to try to check all of the dwarf-related output.

Making the tarballs more reproducible sounds great.

The test fails on Mac, see e.g. https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8863756117615904432/+/steps/package_clang/0/stdout

I don't think it makes sense to try to emit the closure until we have the ctor definition. I'll update the patch to handle this.

Ok. That's a bit different than what MSVC is doing. It generates a closure constructor even if there is only a declaration. But I guess we are not aligning specifically with MSVC!
Look good.

In D87544#2339299, @dmajor wrote:

This caused a regression when calling through function pointers in a certain way as seen in sqlite3: https://bugs.llvm.org/show_bug.cgi?id=47905

Handling the decl only case.

clang.exe -c test.cpp
Assertion failed: !hasUninstantiatedDefaultArg() && "Default argument is not yet instantiated!", file D:\IUSERS\zahiraam\llorg_ws\ws1\llvm\clang\lib\AST\Decl.cpp, line 2719
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.

I was looking into this today, and sent out what I think is a better approach: https://reviews.llvm.org/D91089

Please take a look.

Addressing comments.

In D87372#2376305, @ki.stfu wrote:

@hans they are ignored because Clang behaves like MSVC with these options
enabled (C++ standard conformance mode)

I'm not sure ignoring these is the right thing to do. Maybe they should be "Unsupported but parsed options" instead?

lgtm

lgtm

lgtm

Sorry for the late follow-up, but this caused big problems in Chromium.

Looks reasonable to me.

lgtm thanks!

Thanks for fixing, but can you please elaborate a little since I'm not familiar with the details here:

Follow-up to fix the Windows build, https://github.com/llvm/llvm-project/commit/a7acee89d68473183cc5021d952a56cdf0ae27d3

This broke Chromium's build, causing it to fail with
"Cannot pop empty stack!" from one of the backend passes.