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

[Hans Wennborg]

On Tue, Aug 4, 2015 at 4:24 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
> 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.

Lang, I think this is up to you. Do you want to change this, or should
I just go ahead and merge it?

Cheers,
Hans

>>>> 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)...