- Writes ".debug_XXX" into corresponding WASM custom sections
- Writes relocation records into "reloc.DEBUG" section
Details
- Reviewers
sbc100 - Commits
- rGd177ab2a5f5a: [WebAssembly] Add support for debug (DWARF) sections
rG6a31a0d694a6: [WebAssembly] Write DWARF data into wasm object file
rL331566: [WebAssembly] Add support for debug (DWARF) sections
rLLD331566: [WebAssembly] Add support for debug (DWARF) sections
rL330982: [WebAssembly] Write DWARF data into wasm object file
rL241362: Revert "Refactored ARMTargetInfo in order to use the API of…
Diff Detail
- Repository
- rL LLVM
- Build Status
Buildable 17032 Build 17032: arc lint + arc unit
Event Timeline
lib/MC/WasmObjectWriter.cpp | ||
---|---|---|
406 | No curly braces here. | |
947 | There should be one reloc section for each wasm section that contains relocs. So I think this should generate multiple relocations sections with names like reloc.debug_strings etc.. Each of the reloc sections starts by specifying the target section, then each relocation entry doesn't need to specify the target section. See: https://github.com/WebAssembly/tool-conventions/blob/master/Linking.md Maybe there is some reason why this doesn't work for you? | |
1037 | remove curlys | |
1047 | remove comment |
Record relocatations per custom section
Add section symbols
Add special code, data, custom section relocation records
include/llvm/BinaryFormat/Wasm.h | ||
---|---|---|
150 | Could we just use the the section index here and reuse the existing ElementIndex? (Or at last move the Section inside the union?) | |
lib/MC/WasmObjectWriter.cpp | ||
932 | Is there anything debug specific here? How about writeCustomRelocSections()? Or better still factor out the duplicate code in writeCodeRelocSection and writeDataRelocSection in single writeRelocSection(Name, Relocations);, then call that in a loop? | |
lib/Object/WasmObjectFile.cpp | ||
1076 | ELF uses ST_Debug for STT_SECTION symbols. (see include/llvm/Object/ELFObjectFile.h) |
include/llvm/BinaryFormat/Wasm.h | ||
---|---|---|
150 |
I moved it here due to compiler complaining about StringRef not being a trivial type. |
Changes std::unordered_set to StringSet
Comment for patching XXX_OFFSET_I32 relocations
Minor fixes for reading section symbols
lib/MC/WasmObjectWriter.cpp | ||
---|---|---|
1068 |
That's correct -- added this in a comment |
lib/DebugInfo/DWARF/DWARFContext.cpp | ||
---|---|---|
1146 ↗ | (On Diff #140889) | Debug print should go back out or go behind DEBUG() |
lib/MC/WasmObjectWriter.cpp | ||
52 | If we always reserve 5 bytes for the section size, couldn't the payload offset always be size offset + 5? We don't rewrite those in MC, right? | |
139 | MCSection *Section (here and 141) |
lib/MC/WasmObjectWriter.cpp | ||
---|---|---|
52 | Would you like to express it as invariant / debug only field, and add related asserts in the code? |
lib/MC/WasmObjectWriter.cpp | ||
---|---|---|
52 | I think are you right Derek. Its not strictly related to this change but i guess we never really needed ContentsOffset. |
include/llvm/ObjectYAML/WasmYAML.h | ||
---|---|---|
143 | Can we reuse the existing name field here? i.e. can section symbols avoid storing a name at all? Or how about dropping SectionType and simply using ElementIndex to store the index of the section as it appears in the file. That way there is no ambiguity (e.g. when two custom sections have the same name) and you don't need the SectionName field, since it can be looked up by finding the corresponding section. I know we don't really have section index space yet, but I think it might make sense to use that here. |
include/llvm/ObjectYAML/WasmYAML.h | ||
---|---|---|
143 | The decision to go with such section reference was made based on the pattern we used in the "reloc."-section header. Will this bring some inconsistency in the linker spec? |
include/llvm/Object/RelocVisitor.h | ||
---|---|---|
323 | Where is this RelocVistor change used? Can this be a separate change perhaps? | |
lib/MC/WasmObjectWriter.cpp | ||
175 | How about just a bool here? isDebug? | |
490 | Could we move this down the end of the function and just have an .isMetadata() before the final else? Do we need the above error? | |
lib/Object/WasmObjectFile.cpp | ||
1083 | Seems like ELF always return ST_Debug for STT_SECTION type symbols: ELFObjectFile.h:536 | |
1237 | Is this needed as part of this change? On thing to note is that Rel.Index is not always a symbol. For the TYPE_INDEX relocation types its a type index. We might want to revisit this in order to be consistent. |
When I patched this change in I got a failure in test/tools/llvm-objdump/wasm.txt
Also, can you update the test/MC/WebAssembly/debug-info.ll test? In my local version I changed the first line to
; RUN: llc -filetype=obj %s -o - | llvm-readobj -r -s | FileCheck %s
Then added the entire output of readobj as CHECK-NEXT: lines.
lib/Object/WasmObjectFile.cpp | ||
---|---|---|
1174 | I think you can revert this part once https://reviews.llvm.org/D45579 lands |
lib/MC/WasmObjectWriter.cpp | ||
---|---|---|
490 | Yes. But I adjusted named symbols check to ignore R_WEBASSEMBLY_*_OFFSET_I32: |
include/llvm/Object/RelocVisitor.h | ||
---|---|---|
323 | I guess we should add a test for llvm-dwarfdump too? |
Both test files can have the "Debug information is currently not supported" comment remove now I guess?
lib/MC/WasmObjectWriter.cpp | ||
---|---|---|
179 | Looks like this field isn't used. Remove? | |
402–408 | Make this into a single condition? In my simple test it looks like ".comment" is the only other section.. maybe we should allow that and just remove this condition? Or that can be a followup CL. | |
1250 | Can we check for isMetadata here as well and perhaps copy the TODO about allowing all metadata sections from above? | |
1256 | Is this what ELF calls these? I thought it was just ".data" etc. perhaps you can just use SectionName for this symbol name since it already starts with "." so can't conflict with a C/C++ symbol? |
include/llvm/ObjectYAML/WasmYAML.h | ||
---|---|---|
141 | Just reuse ElementIndex? |
include/llvm/Object/RelocVisitor.h | ||
---|---|---|
339 | Presumably we should be returning something useful here? Looks like COFF and Mach-O just "return Value" and don't see "HasError" in the success case. | |
lib/ObjectYAML/WasmYAML.cpp | ||
408 | I would drop the suffix "Index" since we don't use it above in Function, or Global |
include/llvm/Object/RelocVisitor.h | ||
---|---|---|
339 | Whatever passed as Value to RelocVisitor as SymInfo address (https://github.com/llvm-mirror/llvm/blob/0c191c5e264b8cab92b7fe720a1173f54cd4c9b2/lib/DebugInfo/DWARF/DWARFContext.cpp#L1430) is not suitable for DWARFContext |
LGTM. Lets get this landed!
lib/Object/WasmObjectFile.cpp | ||
---|---|---|
1049 | You can remove this TODO. My understanding is that sections symbols always have value 0. At least on ELF they seem to |
That commit broke lld build.
I have committed the obvious fix, please check that it is indeed the expected fix.
Could we just use the the section index here and reuse the existing ElementIndex? (Or at last move the Section inside the union?)