[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
Fri Jul 25 07:18:44 UTC 2014
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.
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