[DWARFLinker] Handle empty sequences when processing DW_AT_LLVM_stmt_sequence attributes
#132875
Conversation
|
@llvm/pr-subscribers-debuginfo Author: None (alx32) ChangesWe previously assumed that every The newly modified test would have triggered the assert that is being removed in this patch. Full diff: https://github.com/llvm/llvm-project/pull/132875.diff 4 Files Affected:
diff --git a/llvm/lib/DWARFLinker/Classic/DWARFLinker.cpp b/llvm/lib/DWARFLinker/Classic/DWARFLinker.cpp
index f66773ad2e694..ae4cc6d85c120 100644
--- a/llvm/lib/DWARFLinker/Classic/DWARFLinker.cpp
+++ b/llvm/lib/DWARFLinker/Classic/DWARFLinker.cpp
@@ -2311,8 +2311,13 @@ void DWARFLinker::DIECloner::generateLineTableForUnit(CompileUnit &Unit) {
uint64_t OrigStmtSeq = StmtSeq.get();
// 1. Get the original row index from the stmt list offset.
auto OrigRowIter = SeqOffToOrigRow.find(OrigStmtSeq);
- assert(OrigRowIter != SeqOffToOrigRow.end() &&
- "Stmt list offset not found in sequence offsets map");
+ // Check whether we have an output sequence for the StmtSeq offset.
+ // Some sequences are discarded by the DWARFLinker if they are invalid
+ // (empty).
+ if (OrigRowIter == SeqOffToOrigRow.end()) {
+ StmtSeq.set(UINT64_MAX);
+ continue;
+ }
size_t OrigRowIndex = OrigRowIter->second;
// 2. Get the new row index from the original row index.
diff --git a/llvm/test/tools/dsymutil/ARM/stmt-seq-macho.test b/llvm/test/tools/dsymutil/ARM/stmt-seq-macho.test
index b5093ba767894..1dd1f61f1f7fb 100644
--- a/llvm/test/tools/dsymutil/ARM/stmt-seq-macho.test
+++ b/llvm/test/tools/dsymutil/ARM/stmt-seq-macho.test
@@ -41,12 +41,21 @@ ATTRIB int function2_copy2(int a) {
int result = a - 22;
return result;
}
+
+struct logic_error {
+ logic_error(const char* s) {}
+};
+
+struct length_error : public logic_error {
+ __attribute__((noinline)) explicit length_error(const char* s) : logic_error(s) {}
+};
int main() {
int sum = 0;
sum += function2_copy2(3);
sum += function3_copy2(41);
sum += function2_copy1(11);
+ length_error e("test");
return sum;
}
EOF
diff --git a/llvm/test/tools/dsymutil/Inputs/private/tmp/stmt_seq/stmt_seq_macho.exe b/llvm/test/tools/dsymutil/Inputs/private/tmp/stmt_seq/stmt_seq_macho.exe
index 138c418aa37b2..4dd4ee8deb0b4 100755
Binary files a/llvm/test/tools/dsymutil/Inputs/private/tmp/stmt_seq/stmt_seq_macho.exe and b/llvm/test/tools/dsymutil/Inputs/private/tmp/stmt_seq/stmt_seq_macho.exe differ
diff --git a/llvm/test/tools/dsymutil/Inputs/private/tmp/stmt_seq/stmt_seq_macho.o b/llvm/test/tools/dsymutil/Inputs/private/tmp/stmt_seq/stmt_seq_macho.o
index 0da06940a023c..76fba6580055b 100644
Binary files a/llvm/test/tools/dsymutil/Inputs/private/tmp/stmt_seq/stmt_seq_macho.o and b/llvm/test/tools/dsymutil/Inputs/private/tmp/stmt_seq/stmt_seq_macho.o differ
|
There was a problem hiding this comment.
We should avoid checking in binary files by using llvm/utils/update_test_body.py
There was a problem hiding this comment.
For the DWARF linker - this is how all tests are currently: https://github.com/llvm/llvm-project/tree/main/llvm/test/tools/dsymutil/Inputs
So I just followed the existing pattern.
There was a problem hiding this comment.
Actually I just realized llvm/utils/update_test_body.py won't avoid checking in the binaries. But it does provide a standard way to generate these binaries. We should set a good example of using this new tool instead of ad hoc scripts.
There was a problem hiding this comment.
I think there are 3 main approaches here:
- Follow existing pattern (check in binaries + script) - i.e. the current version of the PR
- Switch the test to fully use the
update_test_body.pyapproach and instead of checking in binaries, check in the.yamlof the binary. (Example). - Use a hybrid approach where we use
update_test_body.pyto generate the binaries to check in (I don't think there is currently any test using this method).
For this small fix I went with #1 but I am OK to change it to be either way. I'll let @JDevlieghere as the code owner to decide which approach is preferable.
There was a problem hiding this comment.
I wasn't aware of update_test_body.py and I haven't tried it to generate Mach-Os for dsymutil test, but I'm happy to go with a standardized approach as I agree that the current ad-hoc approach is rather painful.
My preferred approach would be to use yaml for the object files (with yaml2obj) and for the debug map (which you can dump with dsymutil). I think that's the most readable/auditable, but it's also a pain to do by hand. Maybe the sweet spot is to have a script similar to update_test_body.py specifically for the DWARF linker?
Anyway, for this PR I think it's fine to stick with (1) as that's the established way of doing this while we settle on how to go forward. I always (try to) include instructions and source code to regenerate the test so maybe we can use some of the existing ones to test the approach, but that's definitely outside the scope of this PR.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/14989 Here is the relevant piece of the build log for the reference |
We previously assumed that every
DW_AT_LLVM_stmt_sequenceattribute has a corresponding sequence in the processed line table. However, this isn't always true. Some sequences can be removed by the linker if they are empty, as shown here. When an attribute refers to one of these removed sequences, there is no actual sequence for it to match. In such cases, we update the attribute to indicate that it is invalid and does not point to any sequence. This informs readers that the attribute should be ignored.The newly modified test would have triggered the assert that is being removed in this patch.