Page MenuHomePhabricator

[XCOFF/objdump] Fix crash in objdump when trying to use -r with XCOFF binaries
AbandonedPublic

Authored by davidb on Feb 19 2020, 2:43 AM.

Details

Summary

Add a warning and disable -r if specified by the user for now.

Event Timeline

davidb created this revision.Feb 19 2020, 2:43 AM
Herald added a project: Restricted Project. · View Herald Transcript
davidb updated this revision to Diff 245361.Feb 19 2020, 2:45 AM
  • fix space issues

I think XCOFF is a WIP, so many things can break. It may be fine not adding a warning.

I think XCOFF is a WIP, so many things can break. It may be fine not adding a warning.

Is it ok to silently disable Relocations to prevent crashing at least? This becomes an issue in D74824.

It's good that we don't crash, but I'm wondering why not just implement relocation printing for XCOFF? Also, is the intention to not print relocations mixed with disassembly only (i.e. -r + -d/D etc), or also relocation printing without disassembly (i.e. -r on its own)? If the latter, there needs to be a test for that too.

llvm/test/tools/llvm-objdump/xcoff-disassemble-all.test
19

You could avoid the need for this -NOT check, by using CHECK-EMPTY/CHECK-NEXT lines to match between the previous CHECK and the CHECK: disassembly... lines. That way, you don't need to have two identical strings here.

llvm/tools/llvm-objdump/llvm-objdump.cpp
2198

Nit: missing trailing full stop.

I want to understand how this change would help you avoid crashes in D74824.
I think the crash is due to removal of "if (InlineRelocs)" in

if (InlineRelocs)
  RelocMap = getRelocsMap(*Obj);

After this change, you would still call getRelocsMap() no matter Relocations set to false or not, which is still resulting in crash.

FYI, I do have a downstream patch that would implement '-r' for XCOFF format. I could post that asap if that helps. Please let me know.

It's good that we don't crash, but I'm wondering why not just implement relocation printing for XCOFF? Also, is the intention to not print relocations mixed with disassembly only (i.e. -r + -d/D etc), or also relocation printing without disassembly (i.e. -r on its own)? If the latter, there needs to be a test for that too.

I've essentially did the bare minimum required to continue with D74824 where I discovered the failure with -r specified. I suspect that adding relocation printing support would require a lot more effort than it would take for introducing this warning (Which I currently cannot commit to, but was intending to come back to).

I want to understand how this change would help you avoid crashes in D74824.
I think the crash is due to removal of "if (InlineRelocs)" in

if (InlineRelocs)
  RelocMap = getRelocsMap(*Obj);

After this change, you would still call getRelocsMap() no matter Relocations set to false or not, which is still resulting in crash.

Hi Jason, yes, specifying -r results in a crash. The way that I currently have implemented D74824 means that getRelocsMap()is always called, which would result in a crash if objdump was to call it elsewhere.

FYI, I do have a downstream patch that would implement '-r' for XCOFF format. I could post that asap if that helps. Please let me know.

That would be great to get a ready implementation upstream. No rush where D74824 is concerned, I think that requires more work/discussion.

That would be great to get a ready implementation upstream. No rush where D74824 is concerned, I think that requires more work/discussion.

I created a patch at D75131.

davidb added a comment.Apr 8 2020, 8:15 PM

no longer needed after D75131

no longer needed after D75131

You can click "Add Action..." -> "Abandon.." if you no longer need a change. This is also nice for reviewers because the change will disappear from their review queues.

davidb abandoned this revision.Apr 9 2020, 4:08 AM