This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Handle SHT_RISCV_ATTRIBUTES similarly to SHT_ARM_ATTRIBUTES
ClosedPublic

Authored by jrtc27 on Aug 20 2020, 10:35 AM.

Details

Summary

Currently we treat SHT_RISCV_ATTRIBUTES like a normal section and
concatenate all such input sections, yielding invalid output unless only
a single attributes section is present in the input. Instead, pick the
first as with SHT_ARM_ATTRIBUTES. We do not currently need to condition
our behaviour on the contents, unlike Arm. In future, we should both do
stricter validation of the input and merge all sections together to
ensure we have, for example, the full arch string requirement, but this
rudimentary implementation is good enough for most common cases.

Diff Detail

Event Timeline

jrtc27 created this revision.Aug 20 2020, 10:35 AM
jrtc27 requested review of this revision.Aug 20 2020, 10:35 AM
jrtc27 added inline comments.Aug 20 2020, 10:42 AM
lld/ELF/InputFiles.cpp
873

NB: There is no default case for the switch statement so hoisting these out (required since SHT_ARM_ATTRIBUTES and SHT_RISCV_ATTRIBUTES use the same encoding in the arch-specific range) and thus losing the ability to break to the end of the switch shouldn't be a change of behaviour.

jrtc27 updated this revision to Diff 286859.Aug 20 2020, 10:44 AM

Fixed stray non-breaking spaces

MaskRay added inline comments.Aug 20 2020, 4:17 PM
lld/ELF/InputFiles.cpp
871

Keeping the switch may be fine.

case SHT_ARM_ATTRIBUTES:
  assert(SHT_ARM_ATTRIBUTES == SHT_RISCV_ATTRIBUTES);
  if (EM_ARM) {
  } else if (EM_RISCV) {
  } else
    break;

Adding assert(SHT_ARM_ATTRIBUTES == SHT_RISCV_ATTRIBUTES); should be sufficiently clear.

lld/test/ELF/riscv-attributes.s
6

Does llvm-readelf --arch-specific work?

jrtc27 updated this revision to Diff 290084.Sep 5 2020, 7:02 AM

Use llvm-readelf rather than llvm-readobj

They both have the same format for BuildAttributes, but llvm-readelf only emits
that whereas llvm-readobj emits additional information about the file we don't
care about.

jrtc27 marked an inline comment as done.Sep 5 2020, 7:08 AM
jrtc27 added inline comments.
lld/ELF/InputFiles.cpp
871

I considered that but it just feels too dirty... and what happens if another architecture allocates LOPROC+3 to a different kind of section? Then you have completely unrelated code under SHT_ARM_ATTRIBUTES.

MaskRay accepted this revision.Sep 5 2020, 10:34 AM

LGTM.

lld/ELF/InputFiles.cpp
871

what happens if another architecture allocates LOPROC+3 to a different kind of section?

This is probably unlikely. The logic is a bit complex now. So moving it outside can reduce indentation, which seems fine.

This revision is now accepted and ready to land.Sep 5 2020, 10:34 AM