Page MenuHomePhabricator

[DWARF][RISCV] Test resolving of RISC-V relocations
ClosedPublic

Authored by luismarques on Nov 21 2019, 7:40 AM.

Details

Summary

This patch adds an object file (in yaml format) with a synthetic .debug_info section which we use to test that the supported RISC-V relocations are properly resolved.

Diff Detail

Event Timeline

luismarques created this revision.Nov 21 2019, 7:40 AM

This patch is related to D70396. I think it could be committed with D70396.

Add R_RISCV_32_PCREL test.
Tweak comment and hex capitalization.

MaskRay added inline comments.Nov 27 2019, 1:03 PM
llvm/test/tools/llvm-dwarfdump/RISCV/riscv-relocs.yaml
20

For this test, I feel that the opaque content makes the YAML test worse than an assembly test. You can use llvm-readelf -r or llvm-readobj -r to check the presence of intended relocations.

luismarques marked an inline comment as done.Nov 28 2019, 2:40 AM
luismarques added inline comments.
llvm/test/tools/llvm-dwarfdump/RISCV/riscv-relocs.yaml
20

It's not ideal but I don't think we can have an assembly test file we can process with all of the relocations we can resolve. (The goal is to check that they are correctly resolved, not that they are present).

MaskRay added inline comments.Dec 2 2019, 9:54 PM
llvm/test/tools/llvm-dwarfdump/RISCV/riscv-relocs.yaml
20

The opaque and length Content makes the test very difficult to modify. Neither does it explain what each octet does.

Assembly augmented with comments will be easier to understand and modify, e.g.

.byte 8  # Address size
.byte 0  # Segment selector size
.long 0  # Offset entry count
  • Adds instructions for (re)creating/extending the binary blobs inside .debug_info and .debug_abbrev;
  • Simplifies the content of those sections to have only the fields we are interested in.
luismarques marked an inline comment as done.Dec 4 2019, 8:55 AM
luismarques added inline comments.
llvm/test/tools/llvm-dwarfdump/RISCV/riscv-relocs.yaml
20

Changing the test to be an assembly file proved incompatible with adding all of the required relocations (if you know of a way to do it please say so). So instead I added an assembly file as a comment, which documents the binary blobs and allows people to easily recreate and extend the individual relocation tests below. Does that sufficiently address your concerns @MaskRay ?

MaskRay added inline comments.Dec 4 2019, 12:25 PM
llvm/test/tools/llvm-dwarfdump/RISCV/riscv-relocs.yaml
20

Changing the test to be an assembly file proved incompatible with adding all of the required relocations (if you know of a way to do it please say so).

What relocation types cannot be represented by assembly? If there were any, I would think that psABI defined some relocation types that were not actually used in toolchains. If only one or two relocation types that cannot be represented but you do want to test them, how about just creating a separate test?

luismarques marked an inline comment as done.Dec 5 2019, 2:35 AM
luismarques added inline comments.
llvm/test/tools/llvm-dwarfdump/RISCV/riscv-relocs.yaml
20

While some RISC-V relocations you can emit explicitly (e.g. %pcrel_hi) many of them you only get indirectly as the result of filling in some fields in debug sections and so on. Doing that doesn't seem to allow enough control over the contents of the ELF file (e.g. the base value to which relocations are applied on top of). I explored creating a .rela.debug_info section in the assembly file with the relocation entry fields manually specified but the assembler doesn't play nice, so that section isn't recognized as a proper relocation section. So that's what I meant, unlike when you use a yaml object file you can't just say (AFAIK) "have the value X here and apply the relocation Y on top of it". That's why it seems to me that the yaml file is the way to go, but if you know of a way to solve that then I have no problem with changing the test to use just an assembly file.

MaskRay accepted this revision.Dec 5 2019, 11:24 AM
MaskRay added inline comments.
llvm/test/tools/llvm-dwarfdump/RISCV/riscv-relocs.yaml
20

(e.g. the base value to which relocations are applied on top of).

you can't just say (AFAIK) "have the value X here and apply the relocation Y on top of it".

To have a different base value is a sufficiently strong argument to keep it as an opaque 'Content' field in a YAML test. LG!

This revision is now accepted and ready to land.Dec 5 2019, 11:24 AM
This revision was automatically updated to reflect the committed changes.