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

[David Blaikie]

Sent out http://reviews.llvm.org/D11779 to fix the UB in this test.

On Wed, Aug 5, 2015 at 11:51 AM, Hans Wennborg <hans at chromium.org> wrote:

> 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)...
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150805/7970be81/attachment.html>