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

Mon Feb 24 16:00:46 PST 2020

davidb added a comment.

In D74823#1889317 <https://reviews.llvm.org/D74823#1889317>, @jhenderson wrote:

> 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 <https://reviews.llvm.org/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).

In D74823#1889422 <https://reviews.llvm.org/D74823#1889422>, @jasonliu wrote:

> I want to understand how this change would help you avoid crashes in D74824 <https://reviews.llvm.org/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 <https://reviews.llvm.org/D74824> means that `getRelocsMap()`is always called, which would result in a crash if objdump was to call it elsewhere.

In D74823#1889461 <https://reviews.llvm.org/D74823#1889461>, @jasonliu wrote:

> 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 <https://reviews.llvm.org/D74824> is concerned, I think that requires more work/discussion.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74823/new/

https://reviews.llvm.org/D74823