[9] RFR(S): 8049043: Load variable through a pointer of an incompatible type in hotspot/src/share/vm/runtime/sharedRuntimeMath.hpp

[David Chase]

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.  (Aren’t you loving this review process? :-)

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 842 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20140723/d4f6c896/signature.asc>