This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Support debug info for TLS + global in PIC mode
ClosedPublic

Authored by aheejin on Mar 8 2023, 4:16 PM.

Details

Summary

This adds debug info support for

  • thread_local global variables, both in non-PIC and PIC modes
  • (non-thread_local) Global variables in PIC mode

The former needs to read the value from an offset relative to
__tls_base and the latter an offset from __memory_base. The code for
doing this overlaps with some of the existing code to add
__stack_pointer global, so this adds a new member function to add a
a global in TI_GLOBAL_RELOC mode and use it in all three places.

Split DWARF support is currently patchy at best, because the index for
__tls_base is not fixed after dynamic linking. The preexisting split
DWARF support for __stack_pointer relies on that in practice it is
always index 0. This does similar hardcoding for __tls_base and
__memory_base, but __tls_base's index in dynamic linking is not
fixed now (See
https://github.com/llvm/llvm-project/blob/19afbfe33156d211fa959dadeea46cd17b9c723c/lld/wasm/Driver.cpp#L786-L823
for details), TLS + dynamic linking will not work at the moment.

Fixes https://bugs.chromium.org/p/chromium/issues/detail?id=1416702.

Diff Detail

Event Timeline

aheejin created this revision.Mar 8 2023, 4:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2023, 4:16 PM
Herald added subscribers: pmatos, asb, wingo and 4 others. · View Herald Transcript
aheejin requested review of this revision.Mar 8 2023, 4:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2023, 4:16 PM
aheejin updated this revision to Diff 503557.Mar 8 2023, 4:22 PM

Remove TODO

sbc100 added a comment.EditedMar 8 2023, 4:30 PM

When you say ", TLS + dynamic linking will not work at the moment." do you mean just the debug info won't work? Or it won't work at all?

Only debug info won't work. So the devtools DWARF extension (https://chrome.google.com/webstore/detail/cc%20%20-devtools-support-dwa/pdcpmagijalfljmkmjngeonclgbbannb) will display undefined or some garbage value for TLS variables in dynamic linking. But that's the status quo for them anyway.

aheejin updated this revision to Diff 503596.Mar 8 2023, 6:58 PM

Test tidying up

aheejin updated this revision to Diff 503693.Mar 9 2023, 1:28 AM

Add split DWARF tests

aheejin edited the summary of this revision. (Show Details)Mar 9 2023, 1:28 AM
aheejin updated this revision to Diff 503694.Mar 9 2023, 1:32 AM

Revert unnecessary changes

Only debug info won't work. So the devtools DWARF extension (https://chrome.google.com/webstore/detail/cc%20%20-devtools-support-dwa/pdcpmagijalfljmkmjngeonclgbbannb) will display undefined or some garbage value for TLS variables in dynamic linking. But that's the status quo for them anyway.

Just so I'm clear, this is TLS + dynamic linking + split-dwarf, right?

Since the test does actually run that case, maybe it makes sense to put a comment in there about what the output should look like once that case is fixed?

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
496
dschuff added a subscriber: pfaffe.Mar 9 2023, 10:13 AM
aheejin marked an inline comment as done.Mar 9 2023, 5:45 PM

Only debug info won't work. So the devtools DWARF extension (https://chrome.google.com/webstore/detail/cc%20%20-devtools-support-dwa/pdcpmagijalfljmkmjngeonclgbbannb) will display undefined or some garbage value for TLS variables in dynamic linking. But that's the status quo for them anyway.

Just so I'm clear, this is TLS + dynamic linking + split-dwarf, right?

Yes.

Since the test does actually run that case, maybe it makes sense to put a comment in there about what the output should look like once that case is fixed?

That was tricky part when adding the test case. You can see that I put {{[0-9]+}} in place of all global indices. Internally they would contain a relocation symbol that's to be resolved in linker, but at this object file stage they don't mean much; I think they just mean the order of symbols added. So, when you don't use split dwarf, when you use dylink + TLS, because source code adds TLS first, __tls_base is shown as 0 and __memory_base is shown as 1. And if you use dylink witout TLS, __memory_base is shown as 0 instead. So these are not final indices. On the other hand, in case of split dwarf, the indices are fixed, because we hardcoded them, which can be incorrect in the case of dylink + TLS + split dwarf.

If we want to test final indices, we should move this test to lld. I actually considered that, but didn't end up doing it because the change itself doesn't involve anything in the linker. Do you think it's better? Or we leave this as is and only test indices in case of split dwarf? This is not really easy either because we don't know what the real values of those indices in split dwarf should be at this point in the object file.

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
496

lol

aheejin updated this revision to Diff 503993.Mar 9 2023, 5:46 PM
aheejin marked an inline comment as done.

stack_overflow -> stack_pointer

aheejin edited the summary of this revision. (Show Details)Mar 9 2023, 10:12 PM

Ah right. With regular codegen tests we don't have this kind of problem because the assembly output is symbolic; also some tests that test disassembly use objdump -dr which prints relocation info next to instructions that need relocations. But I don't think dwarfdump will do that.
I agree that moving this test to lld doesn't seem great either.

We could test explicitly for relocation information using llvm-readobj (many MC tests do this). I guess for the static linking cases we could assert that there's a relocation against __stack_pointer and in the split dwarf case we could check that there are no relocations. Not sure whether that would be worth it though.

One other question about the test. Did you generate it from a C file? When I've made tests like this in the past I've usually included the C source as a comment at the top to make it easier to regenerate if needed.
Maybe we can land this as-is and see how it works in the debugger, and work on the test more if we improve the code.

aheejin added a comment.EditedMar 10 2023, 6:49 PM

We could test explicitly for relocation information using llvm-readobj (many MC tests do this). I guess for the static linking cases we could assert that there's a relocation against __stack_pointer and in the split dwarf case we could check that there are no relocations. Not sure whether that would be worth it though.

I guess you mean not __stack_pointer but __tls_base and __memory_base, right? This CL doesn't change anything related to __stack_pointer handling. (It refactors the code that handles it, but NFC on that part.)

Yes, I can do that, but AFAIK llvm-readobj doesn't provide any specific info on which global indices contain which reloc. All I can test is that in non-split objects there are relocations referring to __tls_base/__memory_base in .debug_info section, and there are no relocations in dwo files. I guess the latter should be ensured, like, by definition of split DWARF though.

One other question about the test. Did you generate it from a C file?

I originally used a C file, but edited the generated .ll file in several places. Also I removed unrelated debug info a lot; for example, we don't need any line debug info / local variable debug info in this test so I removed all of them to make the test simpler.

When I've made tests like this in the past I've usually included the C source as a comment at the top to make it easier to regenerate if needed.

I used to do that for some tests too, especially the ones that involve control flow (e.g. comments in https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/WebAssembly/exception.ll). For this code, I removed unnecessary parts from the generated code so it wouldn't be exactly the same, but I can come up with C code more or less matches the structure.

aheejin updated this revision to Diff 504330.Mar 10 2023, 8:30 PM

Add reloc test + original C source comment

aheejin updated this revision to Diff 504331.Mar 10 2023, 8:32 PM

Comment fix

dschuff accepted this revision.Mar 13 2023, 10:21 AM

I guess you mean not stack_pointer but tls_base and __memory_base, right?

Yes, sorry.

Yes, I can do that, but AFAIK llvm-readobj doesn't provide any specific info on which global indices contain which reloc. All I can test is that in non-split objects there are relocations referring to tls_base/memory_base in .debug_info section, and there are no relocations in dwo files.

Yes, exactly. We can test part of the behavior we want this way, but not all of it. Hence me not being sure whether it's worth it. But I do like what you've come up with here, it seems pretty straightforward.

I guess the latter should be ensured, like, by definition of split DWARF though.

I'm not actually sure what would happen if we accidentally did something wrong here. e.g. I'm not sure whether the dwo file is created via the same MC mechanism that creates object files (in which case it might be possible or even easy to accidentally create a relocation) or some other mechanism, which case it might not be likely.

I used to do that for some tests too, especially the ones that involve control flow (e.g. comments in https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/WebAssembly/exception.ll). For this code, I removed unnecessary parts from the generated code so it wouldn't be exactly the same, but I can come up with C code more or less matches the structure.

Makes sense, thanks.

This looks good now overall.

This revision is now accepted and ready to land.Mar 13 2023, 10:21 AM
aheejin updated this revision to Diff 504787.Mar 13 2023, 12:02 PM

Fix typo in comment

This revision was landed with ongoing or failed builds.Mar 17 2023, 8:17 PM
This revision was automatically updated to reflect the committed changes.