Skip to content

[DWARFLinker] Handle empty sequences when processing DW_AT_LLVM_stmt_sequence attributes #132875

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 27, 2025
Merged

[DWARFLinker] Handle empty sequences when processing DW_AT_LLVM_stmt_sequence attributes #132875

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions llvm/lib/DWARFLinker/Classic/DWARFLinker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
9 changes: 9 additions & 0 deletions llvm/test/tools/dsymutil/ARM/stmt-seq-macho.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Binary file modified llvm/test/tools/dsymutil/Inputs/private/tmp/stmt_seq/stmt_seq_macho.exe
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should avoid checking in binary files by using llvm/utils/update_test_body.py

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are 3 main approaches here:

  1. Follow existing pattern (check in binaries + script) - i.e. the current version of the PR
  2. Switch the test to fully use the update_test_body.py approach and instead of checking in binaries, check in the .yaml of the binary. (Example).
  3. Use a hybrid approach where we use update_test_body.py to 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.

Copy link
Member

@JDevlieghere JDevlieghere Mar 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Binary file not shown.
Binary file modified llvm/test/tools/dsymutil/Inputs/private/tmp/stmt_seq/stmt_seq_macho.o
View file
Open in desktop
Binary file not shown.