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

[Hans Wennborg via llvm-commits]

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

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