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

Tue Apr 12 10:09:28 PDT 2016

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