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

Tue Aug 11 13:11:21 PDT 2015

On Tue, Aug 11, 2015 at 1:08 PM, Hans Wennborg <hans at chromium.org> wrote:

> I've gone ahead and merged this (r243891) and David's fix (r244644) to
> 3.7 in r244654.
>

Thanks! Sorry for the delays

>
> On Wed, Aug 5, 2015 at 2:20 PM, David Blaikie <dblaikie at gmail.com> wrote:
> > 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/20150811/1fb978a9/attachment.html>