[llvm] r338975 - [ORC] Remove an incorrect use of 'cantFail'.

[David Blaikie via llvm-commits]

On Fri, Aug 17, 2018 at 12:43 PM Lang Hames <lhames at gmail.com> wrote:

> I think that once this functionality is exposed via lli/LLJIT it would be
> reasonable to have a regression test to verify that a bad object files
> trigger an error from the tool code.
>

So this is currently unreachable code? Could it remain an assertion, then,
until it's added - so there's no chance of this slipping through untested?
(this is generally the preferred approach in LLVM, I think - not adding
unreachable/untestable code)


> This will be a New Thing for JIT API testing though: MCJIT and ORCv1 both
> notionally deal with bad object files, but never had tests for them. I
> assume it was just an aversion to checking in binary object-files for
> tests, but in this case we could just test with an empty object file.
>

There are a few projects/areas of LLVM that contend with that htese days -
for instance I think lld's approach is to rely on llvm-mc to assemble
assembly-written test cases into object files to then pass into the linker
for testing. Other areas (like llvm-dwarfdump) have some checked in binary
object files. I'm  undecided on the best approach here - certainly open to
throwing around more ideas in that space.

- Dave


>
> -- Lang.
>
> On Thu, Aug 16, 2018 at 2:19 PM David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>
>> On Thu, Aug 16, 2018 at 11:37 AM Lang Hames <lhames at gmail.com> wrote:
>>
>>> I'm not sure this particular case deserves a test case. Prior to the
>>> change, passing a valid object was a matter of contract.
>>>
>>
>> Seems to me a test case demonstrating that the program behaves as
>> expected when an invalid object is passed, now that that's a valid use-case
>> is merited. Usual sort of LLVM test approach.
>>
>> Whether it's a unit test or an end-to-end test I don't have strong
>> feelings about.
>>
>>
>>> After the change it is a trivial error-propagation, and we don't usually
>>> test them at every level of the stack.
>>>
>>> I would like to see more "bad object" tests for libObject itself, but
>>> that's outside the scope of this patch.
>>>
>>> -- Lang.
>>>
>>> On Mon, Aug 6, 2018 at 8:47 PM David Blaikie <dblaikie at gmail.com> wrote:
>>>
>>>> Test case?
>>>>
>>>> On Sun, Aug 5, 2018 at 4:55 PM Lang Hames via llvm-commits <
>>>> llvm-commits at lists.llvm.org> wrote:
>>>>
>>>>> Author: lhames
>>>>> Date: Sun Aug  5 16:55:35 2018
>>>>> New Revision: 338975
>>>>>
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=338975&view=rev
>>>>> Log:
>>>>> [ORC] Remove an incorrect use of 'cantFail'.
>>>>>
>>>>> This code was moved out from BasicObjectLayerMaterializationUnit,
>>>>> which required
>>>>> the supplied object to be well formed. The getObjectSymbolFlags
>>>>> function does
>>>>> not require a well-formed object, so we have to propagate the error
>>>>> here.
>>>>>
>>>>> Modified:
>>>>>     llvm/trunk/lib/ExecutionEngine/Orc/Layer.cpp
>>>>>
>>>>> Modified: llvm/trunk/lib/ExecutionEngine/Orc/Layer.cpp
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ExecutionEngine/Orc/Layer.cpp?rev=338975&r1=338974&r2=338975&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> --- llvm/trunk/lib/ExecutionEngine/Orc/Layer.cpp (original)
>>>>> +++ llvm/trunk/lib/ExecutionEngine/Orc/Layer.cpp Sun Aug  5 16:55:35
>>>>> 2018
>>>>> @@ -116,8 +116,10 @@ Expected<SymbolFlagsMap> getObjectSymbol
>>>>>    for (auto &Sym : (*Obj)->symbols()) {
>>>>>      if (!(Sym.getFlags() & object::BasicSymbolRef::SF_Undefined) &&
>>>>>          (Sym.getFlags() & object::BasicSymbolRef::SF_Exported)) {
>>>>> -      auto InternedName =
>>>>> -          ES.getSymbolStringPool().intern(cantFail(Sym.getName()));
>>>>> +      auto Name = Sym.getName();
>>>>> +      if (!Name)
>>>>> +        return Name.takeError();
>>>>> +      auto InternedName = ES.getSymbolStringPool().intern(*Name);
>>>>>        auto SymFlags = JITSymbolFlags::fromObjectSymbol(Sym);
>>>>>        if (!SymFlags)
>>>>>          return SymFlags.takeError();
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> 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/20180820/5389f1fe/attachment.html>