it's funny, since this CL doesn't really modify the export behavior.
I'm going to go ahead and revert it though ("revert first and ask questions later" being the usual LLVM practice), until we can figure out what's happening.

This seems to have a bug that we discovered on the emscripten builders where it's exporting the table twice.
The output (from wasm-objdump):

Export[14]:
 - memory[0] -> "memory"
 - table[0] -> "__indirect_function_table"
 - func[3] <__wasm_call_ctors> -> "__wasm_call_ctors"
 - func[5] <main> -> "main"
 - table[0] -> "__indirect_function_table"
 - func[6] <stackSave> -> "stackSave"
 - func[7] <stackRestore> -> "stackRestore"
 - func[8] <stackAlloc> -> "stackAlloc"
 - func[9] <emscripten_stack_init> -> "emscripten_stack_init"
 - func[10] <emscripten_stack_get_free> -> "emscripten_stack_get_free"
 - func[11] <emscripten_stack_get_end> -> "emscripten_stack_get_end"
 - func[12] <saveSetjmp> -> "saveSetjmp"
 - func[13] <testSetjmp> -> "testSetjmp"
 - func[14] <setThrew> -> "setThrew"

I'll see if I can find a more useful reproducer

Hm, that's a good point, I missed the size bit when reading the spec.
DW_OP_stack_value seems to be just sort of the generic terminator; i'm not sure what exactly it would mean if we omitted it? Really I guess it would mean whatever the debugger thinks it does.
I'm not sure whether it would be better to try to shoehorn something in here that almost fits but not quite, or to try to change what we do in the frontend (which could maybe be either changing the type of the parameter to some_struct*, or marking all struct descriptors with DW_AT_calling_convention, or perhaps even just not using byval at all and making the pointer parameter explicit in the frontend.
I'm not sure yet how hard it would be to do those. Do you have a sense of how lldb would react to having OP_deref, or leaving off OP_stack_value? (i.e. would it actually solve the problem, or would it just require a bunch of difficult work on the debugger side?)

@pfaffe does this make sense? (i.e. when a structure is passed by value, it is passed indirectly, and its DW_AT_location ends with a DW_OP_deref to indicate this)

This landed as https://github.com/llvm/llvm-project/commit/cb77e877f8132b885fcac8b7532c58072537b9ed

Ok, and it would change TI_LOCAL_INDIRECT back to TI_LOCAL as it emits DW_OP_deref ?

I looked a little at DW_AT_calling_convention and it looks like clang does use it (e.g. DW_CC_pass_by_reference), even for dwarf 4, for e.g. nontrivially-copyable classes (see e.g. https://github.com/llvm/llvm-project/blob/master/clang/test/CodeGenCXX/debug-info-composite-cc.cpp). In this case though, a function that takes this value is also generated in the IR as a pointer rather than a byval.
In order to make this work we'd probably need to make the contract between a frontend that uses byval and the backend even more obscure (i.e. it would need to mark all the type infos for structs with this attribute in addition to making them passed byval). In other words since it's the backend that decides exactly how a byval IR parameter is passed it makes sense that the backend also sets the debug info accordingly. Assuming that's possible or reasonable, of course; that's why I was asking about what it would take to pass the information through to DwarfExpression::addWasmLocation.

This approach definitely makes for simple implementation. It does seem a little unfortunate (at the dwarf level) that we are inventing something target-specific for a case that could be covered by something generic.
Is it possible just to modify DwarfExpression::addWasmLocation() to slip an extra DW_OP_deref into the expression when the TI kind is indirect? or does that have to be done elsewhere, e.g. in the caller?

In D94140#2481074, @dblaikie wrote:

In D94140#2480989, @aardappel wrote:

@dblaikie Added you for review since this modifies common DWARF code, would love to hear if that is problematic or not. It would seem not, since we're the only users of these TI's it seems, and the new code is generic enough that it could work for future targets using TIs?

Yeah, since you folks are the ones that are using this feature for now (I guess/can you confirm that the TI things were added specifically for WebAssembly?) it's pretty much up to you. For more nuanced review of debug info locations, @aprantl is probably a more useful reference point - the actual optimized debug info variable location tracking, etc, isn't something I've stayed particularly close to.

I found that comdat functions seem to be getting double-included (i.e. not properly excluded) when using --no-gc-sections (independently of whether the object originates from llc or from assembly). I removed the checks that test functions and made them only test the section behavior. We can fix functions in a follow-up.

oops, forgot to publish my comments.

• Simplify; add test for forgetting to specify sectoin
• review
• remove comdat-asm.ll, rebase

• review

OK, added a check for the behavior when the .section directive is missing for a function, and a TODO for the existing problem of honoring the user's specified section name.

fix rebase

Simplify WebAssemblyAsmParser, add test for its behavior, add TODO

This is ready to review now.

rebase, fix typo

I'm going to add assembly support in a separate change. So this change is ready for review; it has the end-to-end test, and when I add assembler support I'll add tests that go from ll -> asm and asm -> YAML

• clang-format

This version basically just tests -fdebug-types-section end-to-end to YAML via comdats. (It probably needs more tests, suggestions are welcome)
I'm thinking it would be good to add assembly syntax so we can test just the MC and Object stuff separately from the debuginfo stuff. Not sure if that should be a separate change?

thanks!

This conversion LGTM.
It occurred to me that I don't know if we have tests that actually cover the conversion that we're removing here, namely LLVM IR -> assembly. e.g. after a really quick search I didn't see an ll test testing extern_weak globals in llvm/test/CodeGen/WebAssembly/ or llvm/test/MC/WebAssembly/. Did I just miss it?

I was curious about emscripten. You mean that this is the mode that links the final wasm? Does emscripten allow users to inject JS functions that are directly imported by the wasm? (I assume we could use that mode for JS functions that are directly packaged by emscripten if we wanted).

What's the use case for 'import-functions' mode after we roll this out?

There are still outstanding comments after the last round, I just haven't had time to get back to it yet and it wasn't on the top of the list. I can bump the priority if you have need for it though?

Just a cross-reference, table-number relocs are being added in D90948

oh, I guess you're referring to the TODO on line 97? Yeah I guess including the case where the operand is a GA instead of an immediate counts as "more cases"...

One other question then: do you know if Debian and/or Ubuntu still have the same support for running x32 programs on the regular x86-64 distribution? (presumably yes, since you aren't changing the existing behavior).
AFAIK clang's current support was developed against Ubuntu, but I haven't tried it in a long time and to my knowledge nobody has submitted any patches for x32 in a while either.

Can you upload the diff with full context (e.g. use diff -U 9999 or use arcanist to upload)?

I think in theory, we could actually keep this optimization if we had a (ULEB?) relocation type with an addend (I think R_WASM_MEMORY_ADDR_LEB would work, actually).
Then we'd emit a load with a reloc against its constant offset field, where the addend would be the frame offset and the symbol would be the operand (similarly to what LadPatImmOff does for ISel)?
Of course if nobody discovered this bug before now it might not be that worthwhile...

LGTM for now.
As I said, I have a feeling we may need to do something more to get the data address space right. But this is fine to avoid crashing.

@sbc100 I found that the cause of the assertion is that
in dwarf 5, the type units apparently go in the .debug_info section (instead of the .debug_type section), and this section already exists (but it was created as a non-comdat).
So when getWasmSection tries to look up an existing section it fails because the group is part of the key. Then it tries to create a new section but that fails because the name is duplicate.

fix diff; it should be against master

can't just getOrCreate symbol without dealing with the section
just disable the test for now as we don't need dw5

use getOrCreate

In D88603#2359554, @dblaikie wrote:

In D88603#2357973, @dschuff wrote:

This broke the bots for some strange reason that didn't reproduce locally. But because it was ~all of them, I probably just did something stupid.

Good to have links to buildbots and/or quotes from buildbots (incase the logs get retired) about the specifics so other folks can chip in/know what went wrong, etc.

This broke the bots for some strange reason that didn't reproduce locally. But because it was ~all of them, I probably just did something stupid.

use GenericSectionID instead of ~0

LGTM modulo Thomas's idea if you think that will work and is an improvement.

So, just to be sure I understand the architecture here:
Previously we use separate longjup_jmpbuf/longjmp and __invoke_{$LLVMSIG} because the lowering pass runs on IR and the IR type system's rules apply (and we don't have wasm signatures there). Now we can get wasm function signatures at AsmPrinter::emitEndOfAsmFile and the only thing that needs to be done is rename them?

LGTM too; I think I created wasm::WasmSignature in service of some other purpose (https://reviews.llvm.org/D52580) and never got around to finishing the job of removing this code...