[llvm] r243891 - [MCJIT] Fix a cast warning in the unit-test introduced in r243589.

[Aaron Ballman]

On Mon, Aug 3, 2015 at 8:47 PM, Hans Wennborg <hans at chromium.org> wrote:
> On Mon, Aug 3, 2015 at 1:32 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
>> On Mon, Aug 3, 2015 at 4:28 PM, Lang Hames <lhames at gmail.com> wrote:
>>> Aaron - I think the API design could be improved, but that's something that
>>> we will need discussion before we change it.
>>
>> Oh, most definitely agreed that discussion is required. :-D
>>
>>>
>>> Dave - Yeah, this is gross, but I'm pretty sure you're right on both counts:
>>> Technically UB, but expected to behave as intended. I'm very open to
>>> changing this if we can come up with something saner.
>>
>> uintptr_t ptr_val = reinterpret_cast<uintptr_t>(&localTestFunc);
>> LLVMAddGlobalMapping(Engine, MappedFn, reinterpret_cast<void *>(ptr_val));
>>
>> I *think* this skirts around the UB, and should still optimize reasonably.
>
> Are there changes planned here, or can I just go ahead and merge
> r243891 as-is to the 3.7 branch?

I would like to see changes before it gets merged, but am uncertain
how others feel.

~Aaron

>
> Thanks,
> Hans
>
>>
>> ~Aaron
>>
>>>
>>> Cheers,
>>> Lang.
>>>
>>>
>>> On Mon, Aug 3, 2015 at 11:36 AM, Aaron Ballman <aaron at aaronballman.com>
>>> wrote:
>>>>
>>>> On Mon, Aug 3, 2015 at 2:25 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>> >
>>>> >
>>>> > On Mon, Aug 3, 2015 at 11:03 AM, Lang Hames <lhames at gmail.com> wrote:
>>>> >>
>>>> >> Author: lhames
>>>> >> Date: Mon Aug  3 13:03:40 2015
>>>> >> New Revision: 243891
>>>> >>
>>>> >> URL: http://llvm.org/viewvc/llvm-project?rev=243891&view=rev
>>>> >> Log:
>>>> >> [MCJIT] Fix a cast warning in the unit-test introduced in r243589.
>>>> >>
>>>> >> Thanks to Aaron Ballman for spotting this.
>>>> >>
>>>> >> Modified:
>>>> >>     llvm/trunk/unittests/ExecutionEngine/MCJIT/MCJITCAPITest.cpp
>>>> >>
>>>> >> Modified: llvm/trunk/unittests/ExecutionEngine/MCJIT/MCJITCAPITest.cpp
>>>> >> URL:
>>>> >>
>>>> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ExecutionEngine/MCJIT/MCJITCAPITest.cpp?rev=243891&r1=243890&r2=243891&view=diff
>>>> >>
>>>> >>
>>>> >> ==============================================================================
>>>> >> --- llvm/trunk/unittests/ExecutionEngine/MCJIT/MCJITCAPITest.cpp
>>>> >> (original)
>>>> >> +++ llvm/trunk/unittests/ExecutionEngine/MCJIT/MCJITCAPITest.cpp Mon
>>>> >> Aug
>>>> >> 3 13:03:40 2015
>>>> >> @@ -514,7 +514,13 @@ TEST_F(MCJITCAPITest, addGlobalMapping)
>>>> >>    buildMCJITOptions();
>>>> >>    buildMCJITEngine();
>>>> >>
>>>> >> -  LLVMAddGlobalMapping(Engine, MappedFn,
>>>> >> reinterpret_cast<void*>(&localTestFunc));
>>>> >> +  union {
>>>> >> +    int (*raw)();
>>>> >> +    void *usable;
>>>> >> +  } functionPointer;
>>>> >> +  functionPointer.raw = &localTestFunc;
>>>> >
>>>> >
>>>> > This is actually UB, but I think it's somewhere in the UB subset that
>>>> > LLVM
>>>> > intentionally preserves the expected behavior of (& I'm not sure if we
>>>> > rely
>>>> > on this in other parts of LLVM/assume that all compilers used to build
>>>> > LLVM
>>>> > support this behavior).
>>>>
>>>> I don't think we should rely on it when alternatives exist that aren't
>>>> UB, such as use of intptr_t and static_assert. I think the API for
>>>> LLVMAddGlobalMapping() is broken if it's requiring a function pointer
>>>> to be convertible to void * (note, I know nothing of
>>>> LLVMAddGlobalMappings, so I may also be off-base)...
>>>>
>>>> ~Aaron
>>>>
>>>> >
>>>> >>
>>>> >> +
>>>> >> +  LLVMAddGlobalMapping(Engine, MappedFn, functionPointer.usable);
>>>> >>
>>>> >>    buildAndRunPasses();
>>>> >>
>>>> >>
>>>> >>
>>>> >> _______________________________________________
>>>> >> llvm-commits mailing list
>>>> >> llvm-commits at cs.uiuc.edu
>>>> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>> >
>>>> >
>>>> >
>>>> > _______________________________________________
>>>> > llvm-commits mailing list
>>>> > llvm-commits at cs.uiuc.edu
>>>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>> >
>>>
>>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits