This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Fix reading addresses in DWARFDebugAddr.
ClosedPublic

Authored by ikudrin on Feb 11 2020, 6:29 AM.

Details

Summary

As addresses in the address tables may have relocations, thus, the relocations should be resolved to read the correct address. That is especially important for targets that use RELA relocations because in that case addends are stored in relocation sections.

Thanks, @jhenderson for spotting this!

Diff Detail

Event Timeline

ikudrin created this revision.Feb 11 2020, 6:29 AM
jhenderson added inline comments.Feb 11 2020, 8:08 AM
llvm/test/tools/llvm-dwarfdump/X86/debug_addr_rela.s
2–3

I'm not sure I entirely follow why you go through the dance you are doing here? Why not just write the YAML directly?

dblaikie added inline comments.Feb 11 2020, 8:26 AM
llvm/test/tools/llvm-dwarfdump/X86/debug_addr_rela.s
2–3

I think the asm/yaml checking isn't needed here (I guess it's an attempt to demonstrate that the values in the section are all zeros, and only by applying the relocation do we get the addend applied - but I don't think the test needs to demonstrate that the bytes are all zeros - the test should just verify that the address printed by dwarfdump includes the addend, however it got there)

So please remove the YAML testing & keep the dwarfdump testing.

jhenderson added inline comments.Feb 11 2020, 8:47 AM
llvm/test/tools/llvm-dwarfdump/X86/debug_addr_rela.s
2–3

FWIW, it _might_ be better to use YAML as input directly, rather than needing to write assembly, but I'm happy to go with whichever is easier to read.

dblaikie added inline comments.Feb 11 2020, 8:55 AM
llvm/test/tools/llvm-dwarfdump/X86/debug_addr_rela.s
2–3

Ah, sure - yeah, I'm not strongly invested in assembly versus YAML. Dealer's choice.

ikudrin updated this revision to Diff 243895.Feb 11 2020, 9:04 AM
  • Removed the YAML part from the test.

Yes, initially, I added it to demonstrate that there is no real address encoded in the raw data of .debug_addr. I feel like I heard about an architecture where addends sometimes were encoded both in relocations and in the relocation target.

dblaikie accepted this revision.Feb 11 2020, 9:11 AM
  • Removed the YAML part from the test.

Yes, initially, I added it to demonstrate that there is no real address encoded in the raw data of .debug_addr. I feel like I heard about an architecture where addends sometimes were encoded both in relocations and in the relocation target.

Yeah, I believe some of those exist - so it's certainly worth it to have looked into that to validate your understanding, but just doesn't make as much sense as a permanent test. (the other aspect: the dump test as-is fails before your change & passes after, so that should be sufficient)

This revision is now accepted and ready to land.Feb 11 2020, 9:11 AM
This revision was automatically updated to reflect the committed changes.