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

Wed Apr 13 08:28:15 PDT 2016

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.

- 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/e51f1c54/attachment.html>