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

[Hans Wennborg]

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?

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