[9] RFR(S): 8049043: Load variable through a pointer of an incompatible type in hotspot/src/share/vm/runtime/sharedRuntimeMath.hpp
Tobias Hartmann
tobias.hartmann at oracle.com
Mon Jul 28 05:57:48 UTC 2014
Thank you, Vladimir.
Best,
Tobias
On 25.07.2014 18:09, Vladimir Kozlov wrote:
> On 7/25/14 12:18 AM, Tobias Hartmann wrote:
>> Hi Vladimir,
>>
>> thanks for the review.
>>
>> On 24.07.2014 17:48, Vladimir Kozlov wrote:
>>> Do we trust that C++ compilers keep declared order of int fields? Is
>>> it in spec? If yes I am fine with your last code.
>>
>> Yes, sentence 13 of Section 9.2. (page 217) of the latest C++
>> standard working draft [1] says:
>>
>> "Nonstatic data members of a (non-union) class with the same access
>> control (Clause 11) are allocated so that later
>> members have higher addresses within a class object. The order of
>> allocation of non-static data members with different
>> access control is unspecified (Clause 11). [...]"
>>
>> We do not have any access control specifiers, so it should be fine.
>
> Good. Thank you for looking to this.
>
> Vladimir
>
>>
>> Thanks,
>> Tobias
>>
>> [1] https://isocpp.org/files/papers/N3797.pdf
>>
>>> > typedef union {
>>> > } DoubleIntConv;
>>> > But that does not really improve code readability... What do you
>>> think?
>>>
>>> It is fine, we do that.
>>>
>>> > I refactored the code and added set_high(..) and set_low(..)
>>> methods (similar to the ones defined in
>>> globalDefinitions.hpp).
>>> >
>>> > New webrev: http://cr.openjdk.java.net/~thartmann/8049043/webrev.03
>>>
>>> I like set_* methods. This webrev.03 looks good.
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 7/24/14 2:47 AM, Tobias Hartmann wrote:
>>>> Hi,
>>>>
>>>> Vladimir, David, thanks for the review. Please see comments inline.
>>>>
>>>>>> Tobias, why you are creating local copy 'conv' of a variable in
>>>>>> all places?
>>>>
>>>> Doing it like this
>>>>
>>>> __HI(x) = (__HI(x)&0x7fffffff)|(__HI(y)&0x80000000);
>>>> return x;
>>>>
>>>> Parfait still complained that a 32 bit value is assigned to x and
>>>> it is then used as a 64 bit value. That's why I had to
>>>> create a local copy of 'conv'.
>>>>
>>>> On 23.07.2014 21:30, David Chase wrote:
>>>>> Vladimir, I think the mistake is instead to include the cast to
>>>>> the type that it already is.
>>>>
>>>> True, the cast is not necessary.
>>>>
>>>> On 23.07.2014 23:12, Vladimir Kozlov wrote:
>>>>> Or you can use union with jlong and don't mess with ENDIAN at all.
>>>>> 64-bit is now main platform now.
>>>>>
>>>>> Vladimir
>>>>
>>>> How do we access the low and high bits then?
>>>
>>>
>>>>
>>>>> On 7/23/14 1:00 PM, David Chase wrote:
>>>>>> It occurred to me that a cleaner way is to put the ifdef in the
>>>>>> definition of the union:
>>>>>>
>>>>>> typedef union {
>>>>>> double d;
>>>>>> struct {
>>>>>> #ifdef VM_LITTLE_ENDIAN
>>>>>> int lo;
>>>>>> int hi;
>>>>>> #else
>>>>>> int hi;
>>>>>> int lo;
>>>>>> #endif
>>>>>> };
>>>>>> } DoubleIntConv;
>>>>>>
>>>>>> # define __HI(x) (x.hi)
>>>>>> # define __LO(x) (x.lo)
>>>>>>
>>>>>> At which point, the continued use of the macro starts to look
>>>>>> perhaps a hair silly.
>>>>>>
>>>>>> I was inspired to write the following test program and compile it
>>>>>> under clang and gcc (but not under Solaris Studio
>>>>>> yet) to see if we could instead just write a stinking static
>>>>>> method (that would be inlined) and at clang -O3 and gcc
>>>>>> -O4, it was indeed inlined. So we could just write clean code
>>>>>> and to heck with the macro.
>>>>
>>>> I like this solution because in my opinion it is the most readable
>>>> one and it is always a good idea to remove ugly
>>>> macros. If the potential slowdown in slowdebug builds is not a
>>>> problem I would like to stick to this.
>>>>
>>>> I removed the macros and changed all parts of the code that used
>>>> them (unfortunately quite a lot).
>>>>
>>>> The parfait warnings in the files sharedRuntimeMath.hpp,
>>>> sharedRutimeTrans.cpp and sharedRuntimeTrigg.cpp were reduced
>>>> by 12.
>>>>
>>>> New webrev: http://cr.openjdk.java.net/~thartmann/8049043/webrev.02
>>>>
>>>> The problem with this solution is that some of the compilers do not
>>>> support anonymous structs. We could name the struct
>>>> and access it via the name, for example like this:
>>>>
>>>> typedef union {
>>>> double d;
>>>> struct {
>>>> #ifdef VM_LITTLE_ENDIAN
>>>> int lo;
>>>> int hi;
>>>> #else
>>>> int hi;
>>>> int lo;
>>>> #endif
>>>> } i;
>>>> } DoubleIntConv;
>>>>
>>>> DoubleIntConv conv = { x };
>>>> int high = conv.i.hi;
>>>>
>>>> But that does not really improve code readability... What do you
>>>> think?
>>>>
>>>> Thanks,
>>>> Tobias
>>>>
>>>>>> (Aren’t you loving this review process? :-)
>>>>
>>>> Yes, thanks for all the testing :)
>>>>
>>>> Best,
>>>> Tobias
>>>>
>>>>>>
>>>>>> We would, however, suffer some slowdown in slowdebug builds.
>>>>>> ----------------------
>>>>>> #define VM_LITTLE_ENDIAN 1
>>>>>> typedef union {
>>>>>> double d;
>>>>>> struct {
>>>>>> #ifdef VM_LITTLE_ENDIAN
>>>>>> int lo;
>>>>>> int hi;
>>>>>> #else
>>>>>> int hi;
>>>>>> int lo;
>>>>>> #endif
>>>>>> };
>>>>>> } DoubleIntConv;
>>>>>>
>>>>>> # define __HI(x) (x.hi)
>>>>>> # define __LO(x) (x.lo)
>>>>>>
>>>>>> static int inline_low(double d) {
>>>>>> DoubleIntConv x = { d } ;
>>>>>> return __LO(x);
>>>>>> }
>>>>>>
>>>>>> static int inline_high(double d) {
>>>>>> DoubleIntConv x = { d } ;
>>>>>> return __HI(x);
>>>>>> }
>>>>>>
>>>>>> int low(double d) {
>>>>>> return inline_low(d);
>>>>>> }
>>>>>>
>>>>>> int high(double d) {
>>>>>> return inline_high(d);
>>>>>> }
>>>>>> ----------------------
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Jul 23, 2014, at 3:37 PM, Vladimir Kozlov
>>>>>> <vladimir.kozlov at oracle.com> wrote:
>>>>>>
>>>>>>> David, I agree with your suggestion to pass union to _HI and
>>>>>>> _LO. The creation of a local union var makes sense then.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Vladimir
>>>>>>>
>>>>>>> On 7/23/14 12:30 PM, David Chase wrote:
>>>>>>>> Vladimir, I think the mistake is instead to include the cast to
>>>>>>>> the type
>>>>>>>> that it already is.
>>>>>>>> I think performing the cast to the union type from a non-union
>>>>>>>> address
>>>>>>>> might get you a warning otherwise.
>>>>>>>> I can confirm that if you write it plainly with the assignment
>>>>>>>> to a
>>>>>>>> correctly typed “conv”
>>>>>>>> that clang (at least) generates good code:
>>>>>>>>
>>>>>>>> union ca {
>>>>>>>> double d;
>>>>>>>> int i[2];
>>>>>>>> };
>>>>>>>>
>>>>>>>> int la(double d) {
>>>>>>>> union ca x = { d } ;
>>>>>>>> return x.i[0];
>>>>>>>> }
>>>>>>>>
>>>>>>>> clang produces (-O3 -S, cleaned up)
>>>>>>>>
>>>>>>>> _la: ## @la
>>>>>>>> pushq %rbp
>>>>>>>> movq %rsp, %rbp
>>>>>>>> movd %xmm0, %rax
>>>>>>>> popq %rbp
>>>>>>>> ret
>>>>>>>> gcc 4.8.1 produces (-O4 -S, cleaned up)
>>>>>>>>
>>>>>>>> la:
>>>>>>>> movsd%xmm0, -8(%rsp)
>>>>>>>> movq-8(%rsp), %rax
>>>>>>>> ret
>>>>>>>>
>>>>>>>> So I think this is generally a good idea, but the macro should be
>>>>>>>> cleaned up to
>>>>>>>> remove the cast that is certainly unnecessary now. E.g.
>>>>>>>>
>>>>>>>> 39 #ifdef VM_LITTLE_ENDIAN
>>>>>>>> 40 # define __HI(x) ((x).i[1])
>>>>>>>> 41 # define __LO(x) ((x).i[0])
>>>>>>>> 42 #else
>>>>>>>> 43 # define __HI(x) (x.i[0])
>>>>>>>> 44 # define __LO(x) (x.i[1])
>>>>>>>> 45 #endif
>>>>>>>>
>>>>>>>> The temporary is only annoying to humans using a last-millenium
>>>>>>>> dialect
>>>>>>>> of C. The code runs fast, and the address-of+dereference in
>>>>>>>> the macro
>>>>>>>> forced it to memory (lacking a lot of alias analysis) anyhow.
>>>>>>>>
>>>>>>>> David
>>>>>>>>
>>>>>>>>
>>>>>>>> On Jul 23, 2014, at 1:58 PM, Vladimir Kozlov
>>>>>>>> <vladimir.kozlov at oracle.com
>>>>>>>> <mailto:vladimir.kozlov at oracle.com>> wrote:
>>>>>>>>
>>>>>>>>> Tobias, why you are creating local copy 'conv' of a variable
>>>>>>>>> in all
>>>>>>>>> places?
>>>>>>>>>
>>>>>>>>> Vladimir
>>>>>>>>>
>>>>>>>>> On 7/23/14 4:12 AM, Tobias Hartmann wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> please review the following patch for JDK- 8049043 (Parfait
>>>>>>>>>> warning).
>>>>>>>>>>
>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8049043
>>>>>>>>>> Webrev: http://cr.openjdk.java.net/~thartmann/8049043/webrev.01/
>>>>>>>>>>
>>>>>>>>>> Problem:
>>>>>>>>>> Parfait issues 6 warnings " Load variable through a pointer
>>>>>>>>>> of an
>>>>>>>>>> incompatible type" because of the __HI(x) and __LO(x) macros
>>>>>>>>>> used in
>>>>>>>>>> copysignA and scalbnA
>>>>>>>>>> (share/vm/runtime/sharedRuntimeMath.hpp) to cast
>>>>>>>>>> between double* and int*.
>>>>>>>>>>
>>>>>>>>>> Solution:
>>>>>>>>>> A union 'DoubleIntConv' is used to access the high and low 32
>>>>>>>>>> bits of a
>>>>>>>>>> double.
>>>>>>>>>> The '# pragma warning( disable: 4748 )' in
>>>>>>>>>> sharedRuntimeTrans.cpp is
>>>>>>>>>> necessary because otherwise the VS compiler fails with the
>>>>>>>>>> following
>>>>>>>>>> warning: "/GS can not protect parameters and local variables
>>>>>>>>>> from local
>>>>>>>>>> buffer overrun because optimizations are disabled in
>>>>>>>>>> function". There
>>>>>>>>>> exists a bug to fix this [1].
>>>>>>>>>>
>>>>>>>>>> Testing:
>>>>>>>>>> JPRT, Parfait (all 6 warnings removed)
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Tobias
>>>>>>>>>>
>>>>>>>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8046696
>>>>>>
>>>>
>>
More information about the hotspot-compiler-dev
mailing list