[llvm-debuginfo-analyzer] Add support for WebAssembly binary format. #82588
Conversation
Add support for the WebAssembly binary format and be able to generate logical views. llvm#69181 The README.txt includes information about how to build the test cases and conversion between .wasm and .wat.
|
@llvm/pr-subscribers-llvm-binary-utilities @llvm/pr-subscribers-backend-webassembly Author: Carlos Alberto Enciso (CarlosAlbertoEnciso) ChangesAdd support for the WebAssembly binary format and be able to generate logical views. The README.txt includes information about how to build the test cases and conversion between .wasm and .wat. Patch is 84.08 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/82588.diff 30 Files Affected:
diff --git a/llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst b/llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst
index a78066a5eea37e..82fad8ce4d5192 100644
--- a/llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst
+++ b/llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst
@@ -16,7 +16,7 @@ DESCRIPTION
binary object files and prints their contents in a logical view, which
is a human readable representation that closely matches the structure
of the original user source code. Supported object file formats include
-ELF, Mach-O, PDB and COFF.
+ELF, Mach-O, WebAssembly, PDB and COFF.
The **logical view** abstracts the complexity associated with the
different low-level representations of the debugging information that
@@ -468,8 +468,9 @@ If the <pattern> criteria is too general, a more selective option can
be specified to target a particular category of elements:
lines (:option:`--select-lines`), scopes (:option:`--select-scopes`),
symbols (:option:`--select-symbols`) and types (:option:`--select-types`).
+
These options require knowledge of the debug information format (DWARF,
-CodeView, COFF), as the given **kind** describes a very specific type
+CodeView), as the given **kind** describes a very specific type
of element.
LINES
@@ -598,7 +599,7 @@ When comparing logical views created from different debug formats, its
accuracy depends on how close the debug information represents the
user code. For instance, a logical view created from a binary file with
DWARF debug information may include more detailed data than a logical
-view created from a binary file with CodeView/COFF debug information.
+view created from a binary file with CodeView debug information.
The following options describe the elements to compare.
@@ -1952,6 +1953,316 @@ The **{Coverage}** and **{Location}** attributes describe the debug
location and coverage for logical symbols. For optimized code, the
coverage value decreases and it affects the program debuggability.
+WEBASSEMBLY SUPPORT
+~~~~~~~~~~~~~~~~~~~
+The below example is used to show the WebAssembly output generated by
+:program:`llvm-debuginfo-analyzer`. We compiled the example for a
+WebAssembly 64-bit target with Clang (-O0 -g --target=wasm64):
+
+.. code-block:: c++
+
+ 1 using INTPTR = const int *;
+ 2 int foo(INTPTR ParamPtr, unsigned ParamUnsigned, bool ParamBool) {
+ 3 if (ParamBool) {
+ 4 typedef int INTEGER;
+ 5 const INTEGER CONSTANT = 7;
+ 6 return CONSTANT;
+ 7 }
+ 8 return ParamUnsigned;
+ 9 }
+
+PRINT BASIC DETAILS
+^^^^^^^^^^^^^^^^^^^
+The following command prints basic details for all the logical elements
+sorted by the debug information internal offset; it includes its lexical
+level and debug info format.
+
+.. code-block:: none
+
+ llvm-debuginfo-analyzer --attribute=level,format
+ --output-sort=offset
+ --print=scopes,symbols,types,lines,instructions
+ test-clang.wasm
+
+or
+
+.. code-block:: none
+
+ llvm-debuginfo-analyzer --attribute=level,format
+ --output-sort=offset
+ --print=elements
+ test-clang.wasm
+
+Each row represents an element that is present within the debug
+information. The first column represents the scope level, followed by
+the associated line number (if any), and finally the description of
+the element.
+
+.. code-block:: none
+
+ Logical View:
+ [000] {File} 'test-clang.wasm' -> WASM
+
+ [001] {CompileUnit} 'test.cpp'
+ [002] 2 {Function} extern not_inlined 'foo' -> 'int'
+ [003] 2 {Parameter} 'ParamPtr' -> 'INTPTR'
+ [003] 2 {Parameter} 'ParamUnsigned' -> 'unsigned int'
+ [003] 2 {Parameter} 'ParamBool' -> 'bool'
+ [003] {Block}
+ [004] 5 {Variable} 'CONSTANT' -> 'const INTEGER'
+ [004] 5 {Line}
+ [004] {Code} 'i32.const 7'
+ [004] {Code} 'local.set 10'
+ [004] {Code} 'local.get 5'
+ [004] {Code} 'local.get 10'
+ [004] {Code} 'i32.store 4'
+ [004] 6 {Line}
+ [004] {Code} 'i32.const 7'
+ [004] {Code} 'local.set 11'
+ [004] {Code} 'local.get 5'
+ [004] {Code} 'local.get 11'
+ [004] {Code} 'i32.store 28'
+ [004] {Code} 'br 1'
+ [004] - {Line}
+ [004] {Code} 'end'
+ [003] 4 {TypeAlias} 'INTEGER' -> 'int'
+ [003] 2 {Line}
+ [003] {Code} 'block unknown_type'
+ [003] {Code} 'i64.mul'
+ [003] {Code} 'throw 127'
+ [003] {Code} 'global.get 0'
+ [003] {Code} 'local.set 3'
+ [003] {Code} 'i64.const 32'
+ [003] {Code} 'local.set 4'
+ [003] {Code} 'local.get 3'
+ [003] {Code} 'local.get 4'
+ [003] {Code} 'i64.sub'
+ [003] {Code} 'local.set 5'
+ [003] {Code} 'local.get 5'
+ [003] {Code} 'local.get 0'
+ [003] {Code} 'i64.store 16'
+ [003] {Code} 'local.get 5'
+ [003] {Code} 'local.get 1'
+ [003] {Code} 'i32.store 12'
+ [003] {Code} 'local.get 2'
+ [003] {Code} 'local.set 6'
+ [003] {Code} 'local.get 5'
+ [003] {Code} 'local.get 6'
+ [003] {Code} 'i32.store8 11'
+ [003] 3 {Line}
+ [003] {Code} 'local.get 5'
+ [003] {Code} 'i32.load8_u 11'
+ [003] {Code} 'local.set 7'
+ [003] 3 {Line}
+ [003] {Code} 'i32.const 1'
+ [003] {Code} 'local.set 8'
+ [003] {Code} 'local.get 7'
+ [003] {Code} 'local.get 8'
+ [003] {Code} 'i32.and'
+ [003] {Code} 'local.set 9'
+ [003] {Code} 'block'
+ [003] {Code} 'block'
+ [003] {Code} 'local.get 9'
+ [003] {Code} 'i32.eqz'
+ [003] {Code} 'br_if 0'
+ [003] 8 {Line}
+ [003] {Code} 'local.get 5'
+ [003] {Code} 'i32.load 12'
+ [003] {Code} 'local.set 12'
+ [003] 8 {Line}
+ [003] {Code} 'local.get 5'
+ [003] {Code} 'local.get 12'
+ [003] {Code} 'i32.store 28'
+ [003] - {Line}
+ [003] {Code} 'end'
+ [003] 9 {Line}
+ [003] {Code} 'local.get 5'
+ [003] {Code} 'i32.load 28'
+ [003] {Code} 'local.set 13'
+ [003] {Code} 'local.get 13'
+ [003] {Code} 'return'
+ [003] {Code} 'end'
+ [003] 9 {Line}
+ [003] {Code} 'unreachable'
+ [002] 1 {TypeAlias} 'INTPTR' -> '* const int'
+
+SELECT LOGICAL ELEMENTS
+^^^^^^^^^^^^^^^^^^^^^^^
+The following prints all *instructions*, *symbols* and *types* that
+contain **'block'** or **'.store'** in their names or types, using a tab
+layout and given the number of matches.
+
+.. code-block:: none
+
+ llvm-debuginfo-analyzer --attribute=level
+ --select-nocase --select-regex
+ --select=BLOCK --select=.store
+ --report=list
+ --print=symbols,types,instructions,summary
+ test-clang.wasm
+
+ Logical View:
+ [000] {File} 'test-clang.wasm'
+
+ [001] {CompileUnit} 'test.cpp'
+ [003] {Code} 'block'
+ [003] {Code} 'block'
+ [003] {Code} 'block unknown_type'
+ [003] {Code} 'i32.store 12'
+ [004] {Code} 'i32.store 28'
+ [003] {Code} 'i32.store 28'
+ [004] {Code} 'i32.store 4'
+ [003] {Code} 'i32.store8 11'
+ [003] {Code} 'i64.store 16'
+
+ -----------------------------
+ Element Total Printed
+ -----------------------------
+ Scopes 3 0
+ Symbols 4 0
+ Types 2 0
+ Lines 62 9
+ -----------------------------
+ Total 71 9
+
+COMPARISON MODE
+^^^^^^^^^^^^^^^
+Given the previous example we found the above debug information issue
+(related to the previous invalid scope location for the **'typedef int
+INTEGER'**) by comparing against another compiler.
+
+Using GCC to generate test-dwarf-gcc.o, we can apply a selection pattern
+with the printing mode to obtain the following logical view output.
+
+.. code-block:: none
+
+ llvm-debuginfo-analyzer --attribute=level
+ --select-regex --select-nocase --select=INTe
+ --report=list
+ --print=symbols,types
+ test-clang.wasm test-dwarf-gcc.o
+
+ Logical View:
+ [000] {File} 'test-clang.wasm'
+
+ [001] {CompileUnit} 'test.cpp'
+ [003] 4 {TypeAlias} 'INTEGER' -> 'int'
+ [004] 5 {Variable} 'CONSTANT' -> 'const INTEGER'
+
+ Logical View:
+ [000] {File} 'test-dwarf-gcc.o'
+
+ [001] {CompileUnit} 'test.cpp'
+ [004] 4 {TypeAlias} 'INTEGER' -> 'int'
+ [004] 5 {Variable} 'CONSTANT' -> 'const INTEGER'
+
+The output shows that both objects contain the same elements. But the
+**'typedef INTEGER'** is located at different scope level. The GCC
+generated object, shows **'4'**, which is the correct value.
+
+There are 2 comparison methods: logical view and logical elements.
+
+LOGICAL VIEW
+""""""""""""
+It compares the logical view as a whole unit; for a match, each compared
+logical element must have the same parents and children.
+
+The output shows in view form the **missing (-), added (+)** elements,
+giving more context by swapping the reference and target object files.
+
+.. code-block:: none
+
+ llvm-debuginfo-analyzer --attribute=level
+ --compare=types
+ --report=view
+ --print=symbols,types
+ test-clang.wasm test-dwarf-gcc.o
+
+ Reference: 'test-clang.wasm'
+ Target: 'test-dwarf-gcc.o'
+
+ Logical View:
+ [000] {File} 'test-clang.wasm'
+
+ [001] {CompileUnit} 'test.cpp'
+ [002] 1 {TypeAlias} 'INTPTR' -> '* const int'
+ [002] 2 {Function} extern not_inlined 'foo' -> 'int'
+ [003] {Block}
+ [004] 5 {Variable} 'CONSTANT' -> 'const INTEGER'
+ +[004] 4 {TypeAlias} 'INTEGER' -> 'int'
+ [003] 2 {Parameter} 'ParamBool' -> 'bool'
+ [003] 2 {Parameter} 'ParamPtr' -> 'INTPTR'
+ [003] 2 {Parameter} 'ParamUnsigned' -> 'unsigned int'
+ -[003] 4 {TypeAlias} 'INTEGER' -> 'int'
+
+The output shows the merging view path (reference and target) with the
+missing and added elements.
+
+LOGICAL ELEMENTS
+""""""""""""""""
+It compares individual logical elements without considering if their
+parents are the same. For both comparison methods, the equal criteria
+includes the name, source code location, type, lexical scope level.
+
+.. code-block:: none
+
+ llvm-debuginfo-analyzer --attribute=level
+ --compare=types
+ --report=list
+ --print=symbols,types,summary
+ test-clang.wasm test-dwarf-gcc.o
+
+ Reference: 'test-clang.wasm'
+ Target: 'test-dwarf-gcc.o'
+
+ (1) Missing Types:
+ -[003] 4 {TypeAlias} 'INTEGER' -> 'int'
+
+ (1) Added Types:
+ +[004] 4 {TypeAlias} 'INTEGER' -> 'int'
+
+ ----------------------------------------
+ Element Expected Missing Added
+ ----------------------------------------
+ Scopes 4 0 0
+ Symbols 0 0 0
+ Types 2 1 1
+ Lines 0 0 0
+ ----------------------------------------
+ Total 6 1 1
+
+Changing the *Reference* and *Target* order:
+
+.. code-block:: none
+
+ llvm-debuginfo-analyzer --attribute=level
+ --compare=types
+ --report=list
+ --print=symbols,types,summary
+ test-dwarf-gcc.o test-clang.wasm
+
+ Reference: 'test-dwarf-gcc.o'
+ Target: 'test-clang.wasm'
+
+ (1) Missing Types:
+ -[004] 4 {TypeAlias} 'INTEGER' -> 'int'
+
+ (1) Added Types:
+ +[003] 4 {TypeAlias} 'INTEGER' -> 'int'
+
+ ----------------------------------------
+ Element Expected Missing Added
+ ----------------------------------------
+ Scopes 4 0 0
+ Symbols 0 0 0
+ Types 2 1 1
+ Lines 0 0 0
+ ----------------------------------------
+ Total 6 1 1
+
+As the *Reference* and *Target* are switched, the *Added Types* from
+the first case now are listed as *Missing Types*.
+
EXIT STATUS
-----------
:program:`llvm-debuginfo-analyzer` returns 0 if the input files were
diff --git a/llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h b/llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h
index a66cf4608823be..f62ae25c10f6db 100644
--- a/llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h
+++ b/llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h
@@ -122,6 +122,48 @@ class LVBinaryReader : public LVReader {
std::unique_ptr<MCContext> MC;
std::unique_ptr<MCInstPrinter> MIP;
+ // https://yurydelendik.github.io/webassembly-dwarf/
+ // 2. Consuming and Generating DWARF for WebAssembly Code
+ // Note: Some DWARF constructs don't map one-to-one onto WebAssembly
+ // constructs. We strive to enumerate and resolve any ambiguities here.
+ //
+ // 2.1. Code Addresses
+ // Note: DWARF associates various bits of debug info
+ // with particular locations in the program via its code address (instruction
+ // pointer or PC). However, WebAssembly's linear memory address space does not
+ // contain WebAssembly instructions.
+ //
+ // Wherever a code address (see 2.17 of [DWARF]) is used in DWARF for
+ // WebAssembly, it must be the offset of an instruction relative within the
+ // Code section of the WebAssembly file. The DWARF is considered malformed if
+ // a PC offset is between instruction boundaries within the Code section.
+ //
+ // Note: It is expected that a DWARF consumer does not know how to decode
+ // WebAssembly instructions. The instruction pointer is selected as the offset
+ // in the binary file of the first byte of the instruction, and it is
+ // consistent with the WebAssembly Web API conventions definition of the code
+ // location.
+ //
+ // EXAMPLE: .DEBUG_LINE INSTRUCTION POINTERS
+ // The .debug_line DWARF section maps instruction pointers to source
+ // locations. With WebAssembly, the .debug_line section maps Code
+ // section-relative instruction offsets to source locations.
+ //
+ // EXAMPLE: DW_AT_* ATTRIBUTES
+ // For entities with a single associated code address, DWARF uses
+ // the DW_AT_low_pc attribute to specify the associated code address value.
+ // For WebAssembly, the DW_AT_low_pc's value is a Code section-relative
+ // instruction offset.
+ //
+ // For entities with a single contiguous range of code, DWARF uses a
+ // pair of DW_AT_low_pc and DW_AT_high_pc attributes to specify the associated
+ // contiguous range of code address values. For WebAssembly, these attributes
+ // are Code section-relative instruction offsets.
+ //
+ // For entities with multiple ranges of code, DWARF uses the DW_AT_ranges
+ // attribute, which refers to the array located at the .debug_ranges section.
+ LVAddress WasmOffset = 0;
+
// Loads all info for the architecture of the provided object file.
Error loadGenericTargetInfo(StringRef TheTriple, StringRef TheFeatures);
diff --git a/llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp b/llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp
index 5f82f816dc19d5..16c4fbed1423fb 100644
--- a/llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp
+++ b/llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp
@@ -48,7 +48,7 @@ Error LVReaderHandler::createReader(StringRef Filename, LVReaders &Readers,
return std::make_unique<LVCodeViewReader>(Filename, FileFormatName,
*COFF, W, ExePath);
}
- if (Obj.isELF() || Obj.isMachO())
+ if (Obj.isELF() || Obj.isMachO() || Obj.isWasm())
return std::make_unique<LVELFReader>(Filename, FileFormatName, Obj, W);
}
if (isa<PDBFile *>(Input)) {
diff --git a/llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp b/llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp
index a0cd8b7839cf7a..d219b8abe1ae98 100644
--- a/llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp
+++ b/llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp
@@ -146,6 +146,30 @@ bool LVBinaryReader::getSymbolTableIsComdat(StringRef Name) {
void LVBinaryReader::mapVirtualAddress(const object::ObjectFile &Obj) {
for (const object::SectionRef &Section : Obj.sections()) {
+ LLVM_DEBUG({
+ Expected<StringRef> SectionNameOrErr = Section.getName();
+ StringRef Name;
+ if (!SectionNameOrErr)
+ consumeError(SectionNameOrErr.takeError());
+ else
+ Name = *SectionNameOrErr;
+ dbgs() << "Index: " << format_decimal(Section.getIndex(), 3) << ", "
+ << "Address: " << hexValue(Section.getAddress()) << ", "
+ << "Size: " << hexValue(Section.getSize()) << ", "
+ << "Name: " << Name << "\n";
+ dbgs() << "isCompressed: " << Section.isCompressed() << ", "
+ << "isText: " << Section.isText() << ", "
+ << "isData: " << Section.isData() << ", "
+ << "isBSS: " << Section.isBSS() << ", "
+ << "isVirtual: " << Section.isVirtual() << "\n";
+ dbgs() << "isBitcode: " << Section.isBitcode() << ", "
+ << "isStripped: " << Section.isStripped() << ", "
+ << "isBerkeleyText: " << Section.isBerkeleyText() << ", "
+ << "isBerkeleyData: " << Section.isBerkeleyData() << ", "
+ << "isDebugSection: " << Section.isDebugSection() << "\n";
+ dbgs() << "\n";
+ });
+
if (!Section.isText() || Section.isVirtual() || !Section.getSize())
continue;
@@ -161,8 +185,14 @@ void LVBinaryReader::mapVirtualAddress(const object::ObjectFile &Obj) {
continue;
}
if ((*SectionNameOrErr).equals(".text") ||
+ (*SectionNameOrErr).equals("CODE") ||
(*SectionNameOrErr).equals(".code"))
DotTextSectionIndex = Section.getIndex();
+ // If the object is WebAssembly, update the address offset that
+ // will be added to DWARF DW_AT_* attributes.
+ if (Obj.isWasm()) {
+ WasmOffset = Section.getAddress();
+ }
}
// Process the symbol table.
diff --git a/llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp b/llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp
index 4469092099daca..79428357fa26c1 100644
--- a/llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp
+++ b/llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp
@@ -415,6 +415,8 @@ void LVELFReader::processOneAttribute(const DWARFDie &Die, LVOffset *OffsetPtr,
if (FoundLowPC) {
if (CurrentLowPC == MaxAddress)
CurrentElement->setIsDiscarded();
+ // Consider the case of WebAssembly.
+ CurrentLowPC += WasmOffset;
if (CurrentElement->isCompileUnit())
setCUBaseAddress(CurrentLowPC);
}
@@ -429,10 +431,14 @@ void LVELFReader::processOneAttribute(const DWARFDie &Die, LVOffset *OffsetPtr,
CurrentHighPC = *Address;
...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Add support for the WebAssembly binary format and be able to generate logical views. llvm#69181 The README.txt includes information about how to build the test cases and conversion between .wasm and .wat. - Fix clang-format reported issue.
CarlosAlbertoEnciso
requested review from
dschuff,
pogo59,
adrian-prantl,
dwblaikie,
djtodoro and
aheejin
| ~~~~~~~~~~~~~~~~~~~ | ||
| The below example is used to show the WebAssembly output generated by | ||
| :program:`llvm-debuginfo-analyzer`. We compiled the example for a | ||
| WebAssembly 64-bit target with Clang (-O0 -g --target=wasm64): |
There was a problem hiding this comment.
wasm64 is not standardized yet so it might be best to stick to wasm32 for the example for now.
There was a problem hiding this comment.
Changed to use wasm32.
| LLVM WebAssembly limitations | ||
| ---------------------------- | ||
| LLVM does not support any conversion between the binary format (.wasm) | ||
| and the text format (.wat): |
There was a problem hiding this comment.
True, but llvm does support its own .s format which is very similar to wat. Would that be useful here instead of wat?
There was a problem hiding this comment.
You are correct in relation to the .s format.
All the references to .wat are just for illustrative purposes and the included .wat files are not required or used by any of the tests. I am happy to exclude them.
The llvm-debuginfo-analyzer only supports .wasm.
There was a problem hiding this comment.
A concern that I communicated to Carlos offline is that we should not be committing binaries with no clear way to reproduce them from source. Without the .wat files the .wasm files are opaque binaries. Now, if it's sufficient to provide .c/.cpp source and use Clang to regenerate the .wasm, I'm okay with that, but I don't ever work with WebAssembly so I don't know how we usually do this.
@dwblaikie tends to have opinions about this sort of thing, I'll defer to him.
There was a problem hiding this comment.
I'm fine with including a way to rebuild any wasm binaries, but why not use the .s format, which we use elsewhere in llvm?
We don't currently have any usage or reference to wat in llvm and I think it would consistent to keep it that way.
When writing tests for the wasm binary format we tent to use wither .yaml files that convert to wasm objects using objywaml or .s files that convert to wasm using llvm-mc. Can either of these two formats for for you?
If you are are starting from .c sources that you should be able to simplest way to generate the .s is simply to run clang with -S.
There was a problem hiding this comment.
A concern that I communicated to Carlos offline is that we should not be committing binaries with no clear way to reproduce them from source. Without the .wat files the .wasm files are opaque binaries. Now, if it's sufficient to provide .c/.cpp source and use Clang to regenerate the .wasm, I'm okay with that, but I don't ever work with WebAssembly so I don't know how we usually do this. @dwblaikie tends to have opinions about this sort of thing, I'll defer to him.
@pogo59 Thanks for your valuable input.
There was a problem hiding this comment.
@sbc100 Thanks for your suggestions to use obj2yaml and llvm-mc.
As I have already the object files, I took the option to use obj2yaml.
Uploaded patch:
- Removed the .wasm and .wat files.
- Removed the README.txt as it is irrelevant now.
- Added .yaml files for the tests.
Add support for the WebAssembly binary format and be able to generate logical views. llvm#69181 - Use '--target=wasm32' for documentation and tests.
|
@sbc100 Thanks for your feedback. |
Add support for the WebAssembly binary format and be able to generate logical views. llvm#69181 - Remove the binary files (.wasm) and associated (.wat) used in the tests and replaced them with their YAML versions. - Remove the README as now it is irrelevant.
| Name: mutable-globals | ||
| - Prefix: USED | ||
| Name: sign-ext | ||
| ... |
There was a problem hiding this comment.
Its really up to you, but I think the the .s format might be readable and editable here. But it if you are always planning on using the .cpp as the source of truth I guess it doesn't matter so much?
I guess maybe using the .s format means depending on a little more of llvm?
If the .cpp files are the source of truth here perhaps include a readme/script for how to regenerate the yaml files?
Add support for the WebAssembly binary format and be able to generate logical views. llvm#69181 - Remove the YAML files used in the tests and replace them with the .s files. - Add the README.txt with instructions on how to generate the .s and .wasm files from the .cpp.
|
I don't have any preference between using
|
Add support for the WebAssembly binary format and be able to generate logical views. llvm#69181 - Update the 'WasmOffset' only when the current section is 'CODE'. In all the other cases, its value is zero.
|
Updated patch:
|
Add support for the WebAssembly binary format and be able to generate logical views. llvm#69181 - Add the '%t' prefix to the generated 'wasm' files to avoid any race condition, as there are tests that use the same generated 'wasm'.
|
Updated patch:
|
| (related to the previous invalid scope location for the **'typedef int | ||
| INTEGER'**) by comparing against another compiler. | ||
|
|
||
| Using GCC to generate test-dwarf-gcc.o, we can apply a selection pattern |
There was a problem hiding this comment.
copypasta? (gcc doesn't actually support wasm, right? have you tried a use case like this?)
There was a problem hiding this comment.
You are correct; GCC does not support wasm.
What the llvm-debuginfo-analyzer does, is to process the binary file created by GCC and creates its logical view (scopes, symbols, types, lines, etc).
Note: The use cases described in the documentation, are used as tests for the tool (DWARD, COFF and WebAssembly) (llvm/test/tools/llvm-debuginfo-analyzer)
| @@ -415,6 +415,8 @@ void LVELFReader::processOneAttribute(const DWARFDie &Die, LVOffset *OffsetPtr, | |||
| if (FoundLowPC) { | |||
| if (CurrentLowPC == MaxAddress) | |||
| CurrentElement->setIsDiscarded(); | |||
| // Consider the case of WebAssembly. | |||
There was a problem hiding this comment.
Should the name of this file/class be LVDWARFReader instead of ELF?
There was a problem hiding this comment.
That is a very good point.
I think LVDWARFReader would match the another reader LVCodeViewReader as they will reflect the type of debug information format that they are parsing.
@dwblaikie, @adrian-prantl, @pogo59: any thoughts on this proposal?
There was a problem hiding this comment.
I agree. Although the renaming should be done as its own separate NFC patch.
There was a problem hiding this comment.
I will prepare a separate NFC patch with the following changes:
Filename change: LVElfReader.cpp[h] -> LVDWARFReader.cpp[h]
Class name change: LVELFReader -> LVDWARFReader
There was a problem hiding this comment.
Does this mean this whole file will be renamed, or only the wasm-related parts will be spun off as a new file? The comment looks like the whole file/class will be renamed.. Then does this class not support ELF anymore? The file heading says it supports ELF/MachO. (Should we update that too?)
| #ifndef SUITE_DEFINITIONS_H | ||
| #define SUITE_DEFINITIONS_H | ||
|
|
||
| #ifdef _MSC_VER |
There was a problem hiding this comment.
since this is input just for testing, the compiler will always be clang and never MSVC. we could probably just write these definitions directly into the test. unless the test is shared with other compilers or platforms?
There was a problem hiding this comment.
All the test cases (.cpp) used by the llvm-debuginfo-analyzer for the WebAssembly are described in detail in the tool documentation.
We used GCC and MSVC to create binaries using those .cpp files that are used for the DWARF and COFF testing.
I included the definitions.h just to keep consistency between documentation and tests (binaries and sources).
|
Thanks, this looks generally good, I just had one other general question: it looks like in this PR you've added a bunch of tests for generic DWARF issues (presumably these have just been copied over from ELF tests?). This makes sense as a way to bootstrap wasm support and verify that its behavior is as expected, but it probably doesn't make sense to duplicate all of the ELF/DWARF tests, since the implementation can hopefully always be shared. For wasm it probably makes more sense to focus on trying to catch ways that they might diverge. Is that how you're thinking about it? Thanks for doing this btw! A while back @aheejin were thinking that it would be nice to have this for wasm, but never got around to trying to make it work! |
|
I can see that may be the added tests seems like a duplication of the tests for the generic There is another point that I think is worth to consider. The specific tests included for the I would suggest for the initial bootstrap WebAssembly support to keep them, unless that there are another issues like extra testing time, etc. Once the initial support is in place, to work with the WebAssembly people and explore areas that are important to check for divergence. |
|
yeah that makes sense; to clarify I'm not asking you to remove these tests. I just don't think it's necessary to keep new ELF tests in lockstep with wasm going forward. |
|
Totally agree with you. Thanks. |
|
@dschuff Thanks very much for your review and approval. |
|
@sbc100 Do you have any additional comments? Thanks. |
| llvm-debuginfo-analyzer --attribute=level,format | ||
| --output-sort=offset | ||
| --print=scopes,symbols,types,lines,instructions | ||
| test-clang.wasm |
There was a problem hiding this comment.
Nit: In LLVM clang -c emits .o and after wasm-ld we usually create .wasm. Given that the wasm file looks like a single object, would it be better to use .o here and below?
There was a problem hiding this comment.
Good point. Changing .wasm to .o.
| ---------------------------------------- | ||
| Element Expected Missing Added | ||
| ---------------------------------------- | ||
| Scopes 4 0 0 | ||
| Symbols 0 0 0 | ||
| Types 2 1 1 | ||
| Lines 0 0 0 | ||
| ---------------------------------------- | ||
| Total 6 1 1 |
There was a problem hiding this comment.
The table looks the same as the previous one after the reference and the target switched. Is that correct?
There was a problem hiding this comment.
The results are correct.
They match the case described in: https://llvm.org/docs/CommandGuide/llvm-debuginfo-analyzer.html#comparison-mode
In the case of WebAssembly vs DWARF, the number of missing and added types is the same:
(1) Missing Types:
-[004] 4 {TypeAlias} 'INTEGER' -> 'int'
(1) Added Types:
+[003] 4 {TypeAlias} 'INTEGER' -> 'int'
| // locations. With WebAssembly, the .debug_line section maps Code | ||
| // section-relative instruction offsets to source locations. | ||
| // | ||
| // EXAMPLE: DW_AT_* ATTRIBUTES |
There was a problem hiding this comment.
| // EXAMPLE: DW_AT_* ATTRIBUTES | |
| // EXAMPLE: DW_AT_* ATTRIBUTES |
There was a problem hiding this comment.
Removed the extra whitespace.
| @@ -415,6 +415,8 @@ void LVELFReader::processOneAttribute(const DWARFDie &Die, LVOffset *OffsetPtr, | |||
| if (FoundLowPC) { | |||
| if (CurrentLowPC == MaxAddress) | |||
| CurrentElement->setIsDiscarded(); | |||
| // Consider the case of WebAssembly. | |||
There was a problem hiding this comment.
Does this mean this whole file will be renamed, or only the wasm-related parts will be spun off as a new file? The comment looks like the whole file/class will be renamed.. Then does this class not support ELF anymore? The file heading says it supports ELF/MachO. (Should we update that too?)
| // | ||
| // For entities with multiple ranges of code, DWARF uses the DW_AT_ranges | ||
| // attribute, which refers to the array located at the .debug_ranges section. | ||
| LVAddress WasmOffset = 0; |
There was a problem hiding this comment.
How about "WasmCodeSectionOffset`? Little more verbose but I feel it less confusing.
There was a problem hiding this comment.
I think it is a good choice.
| // Don't add the WebAssembly offset if we have seen a DW_AT_low_pc. | ||
| CurrentHighPC = | ||
| (FoundLowPC ? CurrentLowPC - WasmOffset : CurrentLowPC) + *Offset; |
There was a problem hiding this comment.
Why is this? Can you elaborate?
There was a problem hiding this comment.
When we detect the DW_AT_low_pc, the CurrentLowPC is updated to reflect the WebAssembly code section offset.
case dwarf::DW_AT_low_pc:
FoundLowPC = true;
...
if (FoundLowPC) {
...
// Consider the case of WebAssembly.
CurrentLowPC += WasmOffset;
...
}
break;
When we detect the DW_AT_high_pc:
case dwarf::DW_AT_high_pc:
...
// Don't add the WebAssembly offset if we have seen a DW_AT_low_pc.
CurrentHighPC =
(FoundLowPC ? CurrentLowPC - WasmOffset : CurrentLowPC) + *Offset;
...
// Consider the case of WebAssembly.
CurrentHighPC += WasmOffset;
...
break;
Basically if we have seen a DW_AT_low_pc, the CurrentLowPC is already increased by the WasmOffset value; so for the calculation of CurrentHighPC the value of CurrentLowPC must be the original value (no addendums).
Added the following comment to the code:
// Don't add the WebAssembly offset if we have seen a DW_AT_low_pc, as
// the CurrentLowPC has already that offset added. Basically, use the
// original DW_AT_loc_pc value.
There was a problem hiding this comment.
Please correct me where I'm mistaken.
Whether CurrentLowPC already contains the WasmOffset or not, CurrentHighPC should be higher than CurrentLowPC, right? The comment says
// High PC is an offset from LowPC.So if the high PC is 50 and the low PC is 40, the offset should be 10. (without WasmOffset)
Now add the WasmOffset. Suppose WasmOffset is 100, and CurrentLowPC is 100 + 40 = 140. And if we want to calculate CurrentHighPC, I think it still should be just
CurrentHighPC = CurrentLowPC + *Offset;So that CurrentHighPC is CurrentLowPC (140) + Offset (10) = 150.
But this equation subtracts WasmOffset from it, so that
CurrentHighPC = (CurrentLowPC - WasmOffset) + *Offset;Here CurrentHighPC will be 140 - 100 + 10 = 50, whereas CurrentLowPC is 140, which is, CurrentLowPC contains WasmOffset, while CurrentHighPC does not.
I think this is strange. But given that your tool works, I must be mistaken somewhere. Can you point out where?
There was a problem hiding this comment.
Your analysis is correct until the line where CurrentHighPC is calculated based on CurrentLowPC.
Referring again to the code for DW_AT_high_pc:
case dwarf::DW_AT_high_pc:
...
if (std::optional<uint64_t> Offset = FormValue.getAsUnsignedConstant())
// Don't add the WebAssembly offset if we have seen a DW_AT_low_pc.
CurrentHighPC =
(FoundLowPC ? CurrentLowPC - WasmOffset : CurrentLowPC) + *Offset;
At this point:
CurrentHighPC is 140 - 100 + 10 = 50, whereas CurrentLowPC is 140
...
// Consider the case of WebAssembly.
CurrentHighPC += WasmOffset;
At this point:
CurrentHighPC will be 50 + 100 = 150, whereas CurrentLowPC is 140 and their offset is 10 (with WasmOffset)
...
break;
I hope this clarify as how the values are calculated. Thanks for your example.
There was a problem hiding this comment.
Ah I see. Then, if we are going to add WasmOffset three lines later, why do we subtract it here?
There was a problem hiding this comment.
The CurrentLowPC (DW_AT_low_pc) always is encoded as an address.
The CurrentHighPC (DW_AT_high_pc) can be encoded as an address or as a constant.
When it is a constant, its value is an offset from the CurrentLowPC.
That is the only case where we use the formula:
CurrentHighPC = (FoundLowPC ? CurrentLowPC - WasmOffset : CurrentLowPC) + *Offset
and we need to check if the CurrentLowPC has been already updated to include the WasmOffset.
If the CurrentHighPC is encoded as an address, we just add the WasmOffset.
Hope this clarify the calculation.
| clang --target=wasm32 -S -g Inputs/pr-46466.cpp -o Inputs/pr-46466-clang.s | ||
| clang --target=wasm32 -S -g Inputs/test.cpp -o Inputs/test-clang.s | ||
|
|
||
| How to generate .wasm from .s |
There was a problem hiding this comment.
Here too. We usually use .o for object files (pre-linking); using .wasm doesn't mean it does not run though.
There was a problem hiding this comment.
Good point. Changing .wasm to .o.
There was a problem hiding this comment.
Thank you for working on this! And sorry for the late reply. I also appreciate your help on my previous questions!
I can't say I understand all the options and tests, but given the quirks of wasm offset being mostly the only thing that needs fixing, this looks nice to me.
The plans are to prepare a separate NFC patch with the following changes: The names will reflect the Debug Information format being processed (DWARF) and not the binary format (ELF, MachO and Wasm).
It will support ELF.
For the time being, we can change the file heading to say something like: |
Add support for the WebAssembly binary format and be able to generate logical views. llvm#69181 Address comments from @aheejin: - Change extension '.wasm' to 'o' for the test and documentation. - Clarify the WebAssembly offset usage when updating the HighPC values. - Notes on future NFC patch to rename the ElfReader.cpp[h] on the ELF and Wasm support.
|
@aheejin Thanks for your valuable comments. Happy to help with the WebAssembly support. |
Oh sorry, I misread it as |
There was a problem hiding this comment.
I can't say I understand all options and test outputs, but the part I understand, mostly the wasm offset code part, looks good to me. (I asked one more question on the existing thread but that question seems minor, and I don't want this to be blocked on that)
Thanks a lot for working on this!
As part of the WebAssembly support work review llvm#82588 Fix clang-format issue.
As part of the WebAssembly support work llvm#82588 As the object files used in the test cases are a single object (just produced by clang without being processed by wasm-ld), it was determined to use .o intead of .wasm. Update the README.txt to reflect that the tool now supports WebAssembly.
As part of the WebAssembly support work #82588 As the object files used in the test cases are a single object (just produced by clang without being processed by wasm-ld), it was determined to use .o intead of .wasm. Update the README.txt to reflect that the tool now supports WebAssembly.
As part of the WebAssembly support work review #82588 It was decided to rename: Files: LVElfReader.cpp[h] -> LVDWARFReader.cpp[h] ELFReaderTest.cpp -> DWARFReaderTest.cpp Class: LVELFReader -> LVDWARFReader The name LVDWARFReader would match the another reader LVCodeViewReader as they will reflect the type of debug information format that they are parsing.
As part of the WebAssembly support work review llvm/llvm-project#82588 It was decided to rename: Files: LVElfReader.cpp[h] -> LVDWARFReader.cpp[h] ELFReaderTest.cpp -> DWARFReaderTest.cpp Class: LVELFReader -> LVDWARFReader The name LVDWARFReader would match the another reader LVCodeViewReader as they will reflect the type of debug information format that they are parsing.
Add support for the WebAssembly binary format and be able to generate logical views.
#69181
The README.txt includes information about how to build the test cases.