[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
Thu Jul 24 09:47:48 UTC 2014
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