[llvm-objdump] Allow -dynamic-reloc on ET_EXEC files

[PkmX]

Summary

Don't exclude ET_EXEC files for -dynamic-reloc/-R as they may also contain dynamic relocations. This matches the behavior of objdump from binutils.

As sh_offset is usually different from sh_addr in ET_EXEC files, this also fixes a bug where section lookup was done by matching the value of DT_REL*/DT_JMPREL (which is a VMA) and the section's file offset.

[PkmX]

Modify dynamic_relocation_sections to return Expected and errors out if input is not ET_EXEC/ET_DYN or when no DYNAMIC section is found, the previous revision would just silently do nothing if given a static executable.

[jhenderson]

Test cases?

include/llvm/Object/ELFObjectFile.h

755–756

I don't think this should be an error. If a client requests dynamic relocations on ET_REL, for example, they should just get no dynamic relocations. The client code should decide whether or not it requires dynamic relocations for there to be no error. Beware that this code could be used by other clients than llvm-objdump that might rely on other behaviour than what you are doing here.

766

I'd prefer if this wasn't auto, as I don't know exactly what the type is (i.e. is it Elf_Shdr or something else).

771–772

This is adding an error where there wasn't one before, which seems outside the scope of this patch. It should be up to the client what to do here.

tools/llvm-objdump/llvm-objdump.cpp

1535

Use the value in Err as the error, to give more meaningful error messages.

[PkmX]

The new patch handles non-dynamic objects logic within llvm-objdump and no longer modifies dynamic_relocation_sections other than making it match on sh_addr. I also added tests for all three cases: ET_DYN (for PIE/shared libraries since there doesn't seem to be an existing test), dynamically linked ET_EXEC and statically-linked ET_EXEC (which should error out).

[jhenderson]

test/tools/llvm-objdump/dynamic-reloc-dyn.test

21 ↗

(On Diff #193929)

Here and below, you don't need to pad the fields with lots of zeroes. just do 0x2D8 or similar. It makes it more readable in my experience.

21–22 ↗

(On Diff #193929)

This is a very arbitrary-looking address and alignment, which might also be a bit fragile. You might find it better to set AddressAlign of 0x400 or 0x1000 and the Address to the same value.

23 ↗

(On Diff #193929)

I don't believe you need this field.

25 ↗

(On Diff #193929)

I'd just leave this Offset as 0.

34 ↗

(On Diff #193929)

You don't need to specify this field.

43–44 ↗

(On Diff #193929)

Similar to above, this Address feels a bit arbitrary. You might want to force the alignment to a round number by e.g. setting the AddressAlign to 0x1000.

test/tools/llvm-objdump/dynamic-reloc-static.test

1 ↗

(On Diff #193929)

Please add a comment to this test about what makes this a static object.

[PkmX]

The test case was reduced from a working executable with unnecessary sections/symbols removed, hence the odd addresses and fields. I will specifically craft a minimal yaml for the dynamic test.

[MaskRay]

I think the error message error: not a dynamic object is a bit inappropriate. GNU objdump reports the error if it cannot find the dynamic section and the BFD flag DYNAMIC (ET_DYN) is not set. This rules looks weird to me as it can just say it cannot find the .dynamic section. What if the file is an ET_DYN but its .dynamic is missing? It may swallow the error message.

static asymbol **
slurp_dynamic_symtab (bfd *abfd)
{
  asymbol **sy = NULL;
  long storage;

  storage = bfd_get_dynamic_symtab_upper_bound (abfd);   //// no dynamic section
  if (storage < 0)
    {
      if (!(bfd_get_file_flags (abfd) & DYNAMIC))      //////// not ET_DYN.
	{
	  non_fatal (_("%s: not a dynamic object"), bfd_get_filename (abfd));
	  exit_status = 1;
	  dynsymcount = 0;
	  return NULL;
	}

I believe we don't have to limit us to ET_EXEC and ET_DYN. If a coredump (ET_CORE) contains .dynamic, we can dump its .dynamic section as well. A probably better error message can just say that the .dynamic section is missing.

[PkmX]

In D60250#1461308, @MaskRay wrote:

I think the error message error: not a dynamic object is a bit inappropriate. GNU objdump reports the error if it cannot find the dynamic section and the BFD flag DYNAMIC (ET_DYN) is not set. This rules looks weird to me as it can just say it cannot find the .dynamic section. What if the file is an ET_DYN but its .dynamic is missing? It may swallow the error message.

Yeah the rule is a bit weird but I think ET_DYN without a .dynamic section is really a corner case. I did try to craft one by hex-editing and ran objdump -R on it, which did successfully *workaround* this condition but then it hit another error in another place.

I believe we don't have to limit us to ET_EXEC and ET_DYN. If a coredump (ET_CORE) contains .dynamic, we can dump its .dynamic section as well. A probably better error message can just say that the .dynamic section is missing.

I agree, we really should just look for .dynamic section in the file regardless of its e_type. Also I don't think coredump records dynamic sections, at least it doesn't seem to on Linux.

[jhenderson]

In D60250#1461461, @PkmX wrote:

In D60250#1461308, @MaskRay wrote:

Yeah the rule is a bit weird but I think ET_DYN without a .dynamic section is really a corner case. I did try to craft one by hex-editing and ran objdump -R on it, which did successfully *workaround* this condition but then it hit another error in another place.

You can use objcopy -R .dynamic to remove the .dynamic without resorting to hex-editing. I've seen developers do some pretty weird "corner case" things and still expect things to work, so it would be good if we can achieve it, assuming the logic isn't too hard. Since it just makes sense to ignore the ELF type entirely, I think this resolves the concern anyway.

[grimar]

tools/llvm-objdump/llvm-objdump.cpp

1533

nit: you can use llvm::any_of, it would be a bit shorter:

return (Elf.getEType() == ELF::ET_DYN || Elf.getEType() == ELF::ET_EXEC) &&
       llvm::any_of(Elf.sections(), [](const ELFSectionRef Sec) {
         return Sec.getType() == ELF::SHT_DYNAMIC;
       });

[PkmX]

• -R now only checks for the presence of SHT_DYNAMIC section.
• Reduce dynamic test case: remove unnecessary fields and use R_X86_64_RELATIVE so we don't even need a symbol table.
• Clarify the test for no SHT_DYNAMIC section case.
• Use llvm::any_of as suggested by @grimar.

[jhenderson]

Looks good, aside from one comment about the tests.

llvm/test/tools/llvm-objdump/dynamic-reloc-dyn.test

11 ↗

(On Diff #195300)

To show that the ELF type isn't important, I'd be tempted to change this to either ET_EXEC, or possibly some arbitrary value e.g. 0x1234. Same with the other test. I think the two tests should use the same ELF type.

[PkmX]

llvm/test/tools/llvm-objdump/dynamic-reloc-dyn.test

11 ↗

(On Diff #195300)

I'm in favor of using previous revision's way of running this test twice with ET_DYN and with s/ET_DYN/ET_EXEC/. The other test can also be modified to also run with ET_DYN to show that llvm-objdump will indeed error out in the absence of dynamic sections even if the ELF is ET_DYN.

I'm not too sure about constructing arbitrary e_type for the test which seems like a precedent in llvm-objdump's tests. Perhaps ET_NONE can be used if we are going this way.

[jhenderson]

llvm/test/tools/llvm-objdump/dynamic-reloc-dyn.test

11 ↗

(On Diff #195300)

ET_NONE works for me too. The main bit I want to emphasise is that the ELF type is irrelevant to the test.

[PkmX]

• Use ET_NONE as e_type in tests.
• Drop the unnecessary dyn suffix in test name.

[Higuoxing]

Seems that this patch has resolved the issue, can we land it?

[MaskRay]

llvm-objdump has some features (-R) which overlap with llvm-readelf (-r). For this one, there are a number of differences:

• No OFFSET TYPE VALUE line.
• No attempt to align VALUE columns.
• Does not test PT_DYNAMIC. objdump -R works without SHT_DYNAMIC. There are a number of changes in llvm-readobj in this area. I don't know how many we want to port/duplicate in llvm-objdump.

This patch is moving toward the right direction and I don't expect it to address the above points, but one unrelated change should be removed and the tests can be organized in a better way...

Requesting changes as I guess the author may be inactive in LLVM...

llvm/include/llvm/Object/ELFObjectFile.h

774 ↗

(On Diff #195346)

Seem unrelated.

llvm/test/tools/llvm-objdump/dynamic-reloc-no-dynamic.test

3 ↗

(On Diff #195346)

-o

Perhaps we should just port some test/tools/llvm-readobj/ELF/dynamic-* here if we are going to improve llvm-objdump -R.

[MaskRay]

The ET_EXEC fix is covered by D110595

include/llvm/Object/ELFObjectFile.h

     }
   }
   for (const Elf_Shdr &Sec : *SectionsOrErr) {
-    if (is_contained(Offsets, Sec.sh_offset))
+    if (is_contained(Offsets, Sec.sh_addr))
       Res.emplace_back(toDRI(&Sec), this);
   }
   return Res;

tools/llvm-objdump/llvm-objdump.cpp

     return;
 
   const auto *Elf = dyn_cast<ELFObjectFileBase>(Obj);
-  if (!Elf || Elf->getEType() != ELF::ET_DYN) {
+  if (!Elf ||
+      (Elf->getEType() != ELF::ET_DYN && Elf->getEType() != ELF::ET_EXEC)) {
     error("not a dynamic object");
     return;
   }