[llvm] r256156 - [llvm-objdump] Use appropriate helper. NFC.

[David Blaikie via llvm-commits]

On Tue, Jan 12, 2016 at 3:20 PM, Davide Italiano <davide at freebsd.org> wrote:

> On Tue, Jan 12, 2016 at 2:54 PM, Davide Italiano <davide at freebsd.org>
> wrote:
> > On Tue, Jan 12, 2016 at 2:24 PM, David Blaikie <dblaikie at gmail.com>
> wrote:
> >>
> >>
> >> On Tue, Jan 12, 2016 at 11:57 AM, Davide Italiano <davide at freebsd.org>
> >> wrote:
> >>>
> >>> On Tue, Jan 12, 2016 at 11:45 AM, David Blaikie <dblaikie at gmail.com>
> >>> wrote:
> >>> >
> >>> >
> >>> > On Mon, Dec 21, 2015 at 6:10 AM, Davide Italiano via llvm-commits
> >>> > <llvm-commits at lists.llvm.org> wrote:
> >>> >>
> >>> >> Author: davide
> >>> >> Date: Mon Dec 21 08:10:54 2015
> >>> >> New Revision: 256156
> >>> >>
> >>> >> URL: http://llvm.org/viewvc/llvm-project?rev=256156&view=rev
> >>> >> Log:
> >>> >> [llvm-objdump] Use appropriate helper. NFC.
> >>> >
> >>> >
> >>> > Test case?
> >>> >
> >>>
> >>> test/tools/llvm-objdump/invalid-input.test should somehow cover this,
> >>> I guess.
> >>
> >>
> >> But there were no changes for that test when this commit was made - so
> it
> >> doesn't seem like it would be testing the change you made (in the sense
> that
> >> it wouldn't catch a regression caused by reverting this patch).
> >>
> >> What would fail if I revert this patch or otherwise broke the
> functionality
> >> you were fixing in this commit?
> >>
> >
> > hmm, I understand you concerns, sorry if it came out as I didn't.
>

Ah, sorry for my misunderstanding.


> > I'll try to build a broken MachO executable and check that in.
> >
> >
>
> OK, it seems code is simply dead (and therefore could be removed).
>
> In particular:
> * If -macho is specified, we already bail out correctly with invalid
> object file much earlier:
>
> (gdb)
> 1480      if (std::error_code EC = BinaryOrErr.getError()) {
> (gdb)
> 1481        errs() << "llvm-objdump: '" << Filename << "': " <<
> EC.message() << ".\n";
> (gdb)
> llvm-objdump: 'hello-macho.bin.2': The file was not recognized as a
> valid object file.
>
> * If the object is valid (but not MachO), that's covered by the if
> statement above the line I changed in the commit.
>
> Both of these are, alas, untested.
> I propose to:
> 1) Add test for these
> 2) Remove the whole else {} condition I previously changed.
>

Based on your description that sounds plausible to me (if you want to be a
bit more sure, you could try adding an assert/llvm_unreachable there either
temporarily or permanently for the case that seems to have become dead -
that way if it turns out not to be dead, we'll hopefully hear about it &
can add new tests/code to cover it then)

Will leave it to you (& Kevin/etc) to decide & just go back to reading this
thread.


>
> Also, CC:ing Kevin Enderby as he knows it much better than I do.
> Kevin, what do you think?
>
> --
> Davide
>
> "There are no solved problems; there are only problems that are more
> or less solved" -- Henri Poincare
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160112/f7e677c7/attachment.html>