<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jan 12, 2016 at 3:20 PM, Davide Italiano <span dir="ltr"><<a href="mailto:davide@freebsd.org" target="_blank">davide@freebsd.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Tue, Jan 12, 2016 at 2:54 PM, Davide Italiano <<a href="mailto:davide@freebsd.org">davide@freebsd.org</a>> wrote:<br>
> On Tue, Jan 12, 2016 at 2:24 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>><br>
>> On Tue, Jan 12, 2016 at 11:57 AM, Davide Italiano <<a href="mailto:davide@freebsd.org">davide@freebsd.org</a>><br>
>> wrote:<br>
>>><br>
>>> On Tue, Jan 12, 2016 at 11:45 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>>> wrote:<br>
>>> ><br>
>>> ><br>
>>> > On Mon, Dec 21, 2015 at 6:10 AM, Davide Italiano via llvm-commits<br>
>>> > <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
>>> >><br>
>>> >> Author: davide<br>
>>> >> Date: Mon Dec 21 08:10:54 2015<br>
>>> >> New Revision: 256156<br>
>>> >><br>
>>> >> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=256156&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=256156&view=rev</a><br>
>>> >> Log:<br>
>>> >> [llvm-objdump] Use appropriate helper. NFC.<br>
>>> ><br>
>>> ><br>
>>> > Test case?<br>
>>> ><br>
>>><br>
>>> test/tools/llvm-objdump/invalid-input.test should somehow cover this,<br>
>>> I guess.<br>
>><br>
>><br>
>> But there were no changes for that test when this commit was made - so it<br>
>> doesn't seem like it would be testing the change you made (in the sense that<br>
>> it wouldn't catch a regression caused by reverting this patch).<br>
>><br>
>> What would fail if I revert this patch or otherwise broke the functionality<br>
>> you were fixing in this commit?<br>
>><br>
><br>
> hmm, I understand you concerns, sorry if it came out as I didn't.<br></span></blockquote><div><br></div><div>Ah, sorry for my misunderstanding.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> I'll try to build a broken MachO executable and check that in.<br>
><br>
><br>
<br>
</span>OK, it seems code is simply dead (and therefore could be removed).<br>
<br>
In particular:<br>
* If -macho is specified, we already bail out correctly with invalid<br>
object file much earlier:<br>
<br>
(gdb)<br>
1480      if (std::error_code EC = BinaryOrErr.getError()) {<br>
(gdb)<br>
1481        errs() << "llvm-objdump: '" << Filename << "': " <<<br>
EC.message() << ".\n";<br>
(gdb)<br>
llvm-objdump: 'hello-macho.bin.2': The file was not recognized as a<br>
valid object file.<br>
<br>
* If the object is valid (but not MachO), that's covered by the if<br>
statement above the line I changed in the commit.<br>
<br>
Both of these are, alas, untested.<br>
I propose to:<br>
1) Add test for these<br>
2) Remove the whole else {} condition I previously changed.<br></blockquote><div><br></div><div>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)<br><br>Will leave it to you (& Kevin/etc) to decide & just go back to reading this thread.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Also, CC:ing Kevin Enderby as he knows it much better than I do.<br>
Kevin, what do you think?<br>
<div class="HOEnZb"><div class="h5"><br>
--<br>
Davide<br>
<br>
"There are no solved problems; there are only problems that are more<br>
or less solved" -- Henri Poincare<br>
</div></div></blockquote></div><br></div></div>