US-Pacific timezone.
User Details
- User Since
- Jan 7 2013, 9:35 AM (419 w, 12 h)
Fri, Jan 15
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?)
Thu, Jan 14
@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)
Tue, Jan 12
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.
Mon, Jan 11
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?
Thu, Jan 7
Wed, Jan 6
Tue, Jan 5
Dec 17 2020
Dec 16 2020
Dec 10 2020
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
Dec 9 2020
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.
Dec 7 2020
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
Dec 4 2020
- 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?
Dec 3 2020
Dec 1 2020
thanks!
Nov 30 2020
Nov 19 2020
Nov 18 2020
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?
Nov 17 2020
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?
Nov 13 2020
Just a cross-reference, table-number relocs are being added in D90948
Nov 3 2020
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"...
Nov 2 2020
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...
Oct 30 2020
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.
Oct 28 2020
@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
Oct 27 2020
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
Oct 14 2020
Oct 7 2020
Oct 2 2020
LGTM modulo Thomas's idea if you think that will work and is an improvement.
Oct 1 2020
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?
Sep 30 2020
Sep 28 2020
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...