Page MenuHomePhabricator

[xray] Add llvm-xray extract support for 32 bit ARM
ClosedPublic

Authored by ianlevesque on May 18 2020, 11:01 PM.

Details

Summary

XRay works on 32-bit ARM but extract didn't support it.

See also another previous attempt in D77858.

Diff Detail

Event Timeline

ianlevesque created this revision.May 18 2020, 11:01 PM
MaskRay added inline comments.May 19 2020, 10:05 AM
llvm/test/tools/llvm-xray/ARM/Inputs/elf32-pic.yaml
139 ↗(On Diff #264798)

Content is not significant. Use Size:

llvm/test/tools/llvm-xray/ARM/extract-instrmap.test
5

You can place the content of elf32-pic.yaml in this file. Then you can delete elf32-pic.yaml

Simplify and combine tests per @MaskRay

ianlevesque marked 2 inline comments as done.May 19 2020, 2:31 PM
MaskRay added inline comments.May 19 2020, 10:57 PM
llvm/lib/XRay/InstrumentationMap.cpp
128

R_ARM_RELATIVE does not need this.

136

Perhaps we can special case R_ARM_* for now. This can avoid a getOptionalAddend change.

I am not sure getOptionalAddend will have more use cases.

llvm/test/tools/llvm-xray/ARM/extract-instrmap.test
74

Please check whether more sections can be dropped. For example, .hash is not read. Many tags in .dynamic are probably not needed. xray_fn_idx may not be needed.

Sorry for letting you do more, but I hope we can get a minimal test case which can be used as a template to improve tests for other architectures.

ianlevesque marked 3 inline comments as done.May 20 2020, 9:36 AM

I'll keep looking at this but @MaskRay if you could point me to something in LLVM that checks whether an addend is present that I can actually access from this module that would be helpful. I added getOptionalAddend as a last resort :(

llvm/lib/XRay/InstrumentationMap.cpp
128

This branch still gets hit for the absolute relocations in the xray_fn_idx section, and with the Expected<> return it asserts in debug mode. I could not figure out a way to detect if the Addend was present from this point in the code, because all of the methods you would use to do so are protected visibilty on the ELF* classes. Can you make a more specific suggestion?

136

I am ok with this, but I'm not sure what the check would look like - the same visibility problem from my other comment applies. Can you suggest something?

llvm/test/tools/llvm-xray/ARM/extract-instrmap.test
74

Sure I can delete even more - I would not cut away xray_fn_idx because it forces that other branch to be taken right now, which exposes another assert().

ianlevesque marked 3 inline comments as done.May 20 2020, 5:29 PM

Remove even more sections from the test object. Special case ARM entirely instead of extending the ELFObject classes.

Oh, do we still have R_ARM_ABS32 relocating xray_instr_map? Aren't them all converted to R_ARM_RELATIVE?

llvm/lib/XRay/InstrumentationMap.cpp
120

If we only have R_ARM_RELATIVE, you can drop the if branch (always false).

ianlevesque marked an inline comment as done.May 20 2020, 7:56 PM
ianlevesque added inline comments.
llvm/lib/XRay/InstrumentationMap.cpp
120

It's relocating the xray_fn_idx with absolute relocations still - I think we can just ignore those here but I'd like to do away with them entirely to improve load time.

Remove unnecessary fallback branch.

ianlevesque edited the summary of this revision. (Show Details)May 20 2020, 9:19 PM

@MaskRay I think this should cover it now. I cut away all the sections I could from the test and eliminated the unused branch and all the ELFObject modifications.

Can I get a re-review on this?

MaskRay accepted this revision.May 27 2020, 11:42 AM

LG, but please remove unneeded re-formatting in this patch.

llvm/lib/XRay/InstrumentationMap.cpp
100

This re-formatting is not needed.

263

The formatting is not needed.

You can ignore clang-format diagnostic for existing problems.

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

This broke check-llvm on virtually all bots on llvm's buildbot for 3h now. Please fix or revert if it takes a while.

MaskRay added a comment.EditedMay 28 2020, 7:17 PM

This broke check-llvm on virtually all bots on llvm's buildbot for 3h now. Please fix or revert if it takes a while.

Thanks for reporting. Fixed by 59ba12994c07d03ac3b628c05c45a834774f9b17

Ugh, cut a bit too deep when removing formatting. Thanks for fixing forward on the test @MaskRay