[llvm] r265815 - [Object] Report an error if .alt_entry is used with ELF or COFF.

Wed Apr 13 10:24:48 PDT 2016

On Wed, Apr 13, 2016 at 8:28 AM, Lang Hames <lhames at gmail.com> wrote:

> Hi Dave,
>
> Sorry - I referenced r265975, but didn't say explicitly: Since that
> commit fixed the issue (by moving this directive into Darwin-specific
> parser code) it also changed these report_fatal_errors back in to
> llvm_unreachables.
>

Ah, makes sense - works for me (based on my aforementioned very cursory
understanding of the code).

>
> - Lang.
>
>
> On Tue, Apr 12, 2016 at 10:09 AM, David Blaikie <dblaikie at gmail.com>
> wrote:
>
>>
>>
>> On Mon, Apr 11, 2016 at 1:13 PM, Lang Hames <lhames at gmail.com> wrote:
>>
>>> Hi Dave,
>>>
>>> 100% code coverage sounds like a good aim, but I'm not sure it applies
>>> here: As of this patch there's no code in the ELF or COFF parser for this
>>> directive. It just goes in the "unsupported directive" bucket. As long as
>>> we're testing one unsupported directive (though to be fair I'm not sure we
>>> are) we've covered all of the relevant code.
>>>
>>
>> What I mean is that the previous unreachable was untested, presumably
>> (because you can't really test an unreachable - it's UB to reach it). So
>> replacing it with a diagnostic (report_fatal_error) means we now have
>> untested code. It'd be nice to test it.
>>
>> I'm not sure how this code could be covered by an existing test case -
>> but I haven't looked at the code more broadly, so I may be missing
>> something.
>>
>> - David
>>
>>
>>>
>>> - Lang.
>>>
>>> On Mon, Apr 11, 2016 at 12:48 PM, David Blaikie <dblaikie at gmail.com>
>>> wrote:
>>>
>>>>
>>>>
>>>> On Mon, Apr 11, 2016 at 12:44 PM, Lang Hames via llvm-commits <
>>>> llvm-commits at lists.llvm.org> wrote:
>>>>
>>>>> Hi Rafael,
>>>>>
>>>>> Yep - this was just a temporary hack. It's been fixed as of r265975.
>>>>>
>>>>> Regarding test cases - what's the bar for something like this. I'd
>>>>> have included a test that the directive was handled correctly on the
>>>>> platforms it's intended for, but testing every (unsupported) directive on
>>>>> every platform seems like overkill.
>>>>>
>>>>
>>>> FWIW, my bar would be "if it's separate code, it probably deserves a
>>>> separate test". (100% block coverage is a pretty low bar for testing (& I
>>>> expect a bit more than that - it's easy to have coverage without
>>>> verification of the right behavior, for example), and, admittedly, not one
>>>> (m)any projects really have, especially with C++ & many implicit blocks
>>>> (maybe that's not so bad with exceptions disabled), but generally something
>>>> to strive for)
>>>>
>>>> - Dave
>>>>
>>>>
>>>>>
>>>>> - Lang.
>>>>>
>>>>> On Sun, Apr 10, 2016 at 8:35 AM, Rafael Espíndola <
>>>>> rafael.espindola at gmail.com> wrote:
>>>>>
>>>>>> Thanks, but:
>>>>>>
>>>>>> * This should have a testcase.
>>>>>> * It is odd to parse and then issue an error. I think the correct fix
>>>>>> is to move the parsing to lib/MC/MCParser/DarwinAsmParser.cpp, that
>>>>>> way we get the natural error on ELF and COFF.
>>>>>>
>>>>>> Cheers,
>>>>>> Rafael
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 8 April 2016 at 13:38, Lang Hames via llvm-commits
>>>>>> <llvm-commits at lists.llvm.org> wrote:
>>>>>> > Author: lhames
>>>>>> > Date: Fri Apr  8 12:38:51 2016
>>>>>> > New Revision: 265815
>>>>>> >
>>>>>> > URL: http://llvm.org/viewvc/llvm-project?rev=265815&view=rev
>>>>>> > Log:
>>>>>> > [Object] Report an error if .alt_entry is used with ELF or COFF.
>>>>>> >
>>>>>> > I'm looking into a better way to do this long-term, but for now at
>>>>>> least don't
>>>>>> > crash.
>>>>>> >
>>>>>> > Modified:
>>>>>> >     llvm/trunk/lib/MC/MCELFStreamer.cpp
>>>>>> >     llvm/trunk/lib/MC/WinCOFFStreamer.cpp
>>>>>> >
>>>>>> > Modified: llvm/trunk/lib/MC/MCELFStreamer.cpp
>>>>>> > URL:
>>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MCELFStreamer.cpp?rev=265815&r1=265814&r2=265815&view=diff
>>>>>> >
>>>>>> ==============================================================================
>>>>>> > --- llvm/trunk/lib/MC/MCELFStreamer.cpp (original)
>>>>>> > +++ llvm/trunk/lib/MC/MCELFStreamer.cpp Fri Apr  8 12:38:51 2016
>>>>>> > @@ -285,7 +285,7 @@ bool MCELFStreamer::EmitSymbolAttribute(
>>>>>> >      break;
>>>>>> >
>>>>>> >    case MCSA_AltEntry:
>>>>>> > -    llvm_unreachable("ELF doesn't support this attribute");
>>>>>> > +    report_fatal_error("ELF doesn't support the .alt_entry
>>>>>> attribute");
>>>>>> >    }
>>>>>> >
>>>>>> >    return true;
>>>>>> >
>>>>>> > Modified: llvm/trunk/lib/MC/WinCOFFStreamer.cpp
>>>>>> > URL:
>>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/WinCOFFStreamer.cpp?rev=265815&r1=265814&r2=265815&view=diff
>>>>>> >
>>>>>> ==============================================================================
>>>>>> > --- llvm/trunk/lib/MC/WinCOFFStreamer.cpp (original)
>>>>>> > +++ llvm/trunk/lib/MC/WinCOFFStreamer.cpp Fri Apr  8 12:38:51 2016
>>>>>> > @@ -107,6 +107,8 @@ bool MCWinCOFFStreamer::EmitSymbolAttrib
>>>>>> >    case MCSA_Global:
>>>>>> >      Symbol->setExternal(true);
>>>>>> >      break;
>>>>>> > +  case MCSA_AltEntry:
>>>>>> > +    report_fatal_error("COFF doesn't support the .alt_entry
>>>>>> attribute");
>>>>>> >    }
>>>>>> >
>>>>>> >    return true;
>>>>>> >
>>>>>> >
>>>>>> > _______________________________________________
>>>>>> > llvm-commits mailing list
>>>>>> > llvm-commits at lists.llvm.org
>>>>>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160413/1a792626/attachment.html>