Page MenuHomePhabricator

ObjectFileELF: Fix symbol lookup in bss section
ClosedPublic

Authored by labath on Apr 24 2017, 7:48 AM.

Details

Summary

If we have symbol information in a separate file, we need to be very
careful about presenting a unified section view of module to the rest of
the debugger. ObjectFileELF had code to handle that, but it was being
overly cautious -- the section->GetFileSize()!=0 meant that the
unification would fail for sections which do not occupy any space in the
object file (e.g., .bss). In my case, that manifested itself as not
being able to display the values of .bss variables properly as the
section associated with the variable did not have it's load address set
(because it was not present in the unified section list).

I test this by making sure the unified section list and the variables
refer to the same section.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Apr 24 2017, 7:48 AM
eugene edited edge metadata.Apr 24 2017, 12:53 PM

Is it really necessary to check in binary ELF files?
I understand that we don't always have a C compiler capable of producing ELF files, but maybe it's ok to skip this test on those platforms.

Is it really necessary to check in binary ELF files?
I understand that we don't always have a C compiler capable of producing ELF files, but maybe it's ok to skip this test on those platforms.

That is something very I am very much trying to avoid, as that means people on those platforms cannot validate their changes. (And I'm not sure if I even want to be running a compiler for a test at this level, as that introduces a level of nondeterminism.)

That said, I do agree we should be carefully about adding lots of binary bloat. I have been trying to put this off until we have more of these, but maybe I could try integrating this with obj2yaml -- it does not seem to preserve program headers, but I don't actually need those for this test.

We don't support running the test suite on Windows with MSVC. We run it
with clang targeting windows instead. So anyone running the test suite on
Windows is already using clang, and we can just specify a linux triple to
get an ELF binary.

You'll need a linker as well, which we do have in the form of lld, but I don't really want to pull in lld for the sake of this test. And I also used objcopy, although for this particular test we could probably do without it.

However, my main question is: can you still call this a unit test if it calls out to a compiler to produce an object file?

labath updated this revision to Diff 96540.Apr 25 2017, 6:04 AM

Use yaml2obj to avoid checking in a binary.

labath updated this revision to Diff 96541.Apr 25 2017, 6:05 AM

Use yaml2obj to avoid checking in a binary.

labath added a subscriber: beanz.

Ok, wiring yaml2obj up was easier than I expected (for cmake, we'll still need to figure out what to do with the xcode build). Let me know what you make of this.

Also adding @beanz, in case he has any thoughts on the subject.

eugene accepted this revision.Apr 25 2017, 11:09 AM

Neatly done!

This revision is now accepted and ready to land.Apr 25 2017, 11:09 AM
zturner edited edge metadata.Apr 25 2017, 11:20 AM

If you look at the source code of yaml2obj, all this boils down to a single call to ELFState<T>::writeELF. If you just link against that, we could avoid even shelling out to an external tool, and the YAML could be a string literal defined inside the unit test cpp file. This would also greatly simplify the xcode build.

Thoughts?

labath planned changes to this revision.Apr 25 2017, 11:58 AM

If you look at the source code of yaml2obj, all this boils down to a single call to ELFState<T>::writeELF. If you just link against that, we could avoid even shelling out to an external tool, and the YAML could be a string literal defined inside the unit test cpp file. This would also greatly simplify the xcode build.

Thoughts?

I like that. Let me see how that goes..

labath requested review of this revision.Apr 26 2017, 4:02 AM

If you look at the source code of yaml2obj, all this boils down to a single call to ELFState<T>::writeELF. If you just link against that, we could avoid even shelling out to an external tool, and the YAML could be a string literal defined inside the unit test cpp file. This would also greatly simplify the xcode build.

Thoughts?

I like that. Let me see how that goes..

Unfortunately, it's not going to be that easy. All of the yaml->elf conversion code lives in tools/yaml2obj, and is not a part of any library any library that we could pull in. Maybe there's a case to be made for making yaml2obj a library, but I am not sure if this is the time to do that.

This revision is now accepted and ready to land.Apr 26 2017, 4:02 AM
zturner accepted this revision.Apr 26 2017, 10:23 AM

Alright, thanks for trying anyway!

emaste added a subscriber: emaste.Apr 30 2017, 1:13 PM
This revision was automatically updated to reflect the committed changes.
fjricci added a subscriber: fjricci.Feb 2 2018, 9:08 AM