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

[Lang Hames via llvm-commits]

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.

- 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/20160411/77ab99a8/attachment.html>