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

[Aaron Ballman]

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.

~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
>> >
>
>