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

[David Blaikie]

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

Yep, this, to the best of my knowledge, is fine (& you don't need the
intermediate variable if you don't want to, just to be clear - you can just
reinterpret_cast<void*>(reinterpret_cast<uintptr_t>(&localTestFunc)))


>
> ~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
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150803/9fde0891/attachment.html>