Re: RFR: 8239001: Hotspot build broken on linux-sparc after 8238281

[David Holmes]

Looks good to me too.

David

On 19/02/2020 6:26 am, Kim Barrett wrote:

On Feb 18, 2020, at 11:24 AM, John Paul Adrian Glaubitz 
 wrote:

Hi Kim!

On 2/18/20 5:07 PM, John Paul Adrian Glaubitz wrote:

I forgot to use a reference type when I tried the definition within the class
which is why it failed for me. I will test the proper approach and post a
patch in a few minutes.


Here's the patch with the changes as you suggested [1].

Adrian


[1] http://cr.openjdk.java.net/~glaubitz/8239001/webrev.03/


Looks good.



--
.''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: RFR: 8239001: Hotspot build broken on linux-sparc after 8238281

[Kim Barrett]

> On Feb 18, 2020, at 11:24 AM, John Paul Adrian Glaubitz 
>  wrote:
> 
> Hi Kim!
> 
> On 2/18/20 5:07 PM, John Paul Adrian Glaubitz wrote:
>> I forgot to use a reference type when I tried the definition within the class
>> which is why it failed for me. I will test the proper approach and post a
>> patch in a few minutes.
> 
> Here's the patch with the changes as you suggested [1].
> 
> Adrian
> 
>> [1] http://cr.openjdk.java.net/~glaubitz/8239001/webrev.03/

Looks good.

> 
> -- 
> .''`.  John Paul Adrian Glaubitz
> : :' :  Debian Developer - glaub...@debian.org
> `. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
>  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: RFR: 8239001: Hotspot build broken on linux-sparc after 8238281

[John Paul Adrian Glaubitz]

Hi Kim!

On 2/18/20 5:07 PM, John Paul Adrian Glaubitz wrote:
> I forgot to use a reference type when I tried the definition within the class
> which is why it failed for me. I will test the proper approach and post a
> patch in a few minutes.

Here's the patch with the changes as you suggested [1].

Adrian

> [1] http://cr.openjdk.java.net/~glaubitz/8239001/webrev.03/

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: RFR: 8239001: Hotspot build broken on linux-sparc after 8238281

[John Paul Adrian Glaubitz]

On 2/18/20 4:55 PM, Kim Barrett wrote:
> Nope.  That unnecessarily pollutes the global namespace with “dummy_regs”.
> 
> The dummy declaration should be in the RegistersForDebugging class as I 
> suggested earlier.
> But you say that “doesn’t work”.  I thought perhaps that meant I’d made was a 
> mistake in the
> code snippet I’d sent, but that seems to work for me.  So what do you mean by 
> “doesn’t work”?

Ah, I completely missed that mail and the code snippet. Sorry.

I forgot to use a reference type when I tried the definition within the class
which is why it failed for me. I will test the proper approach and post a
patch in a few minutes.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: RFR: 8239001: Hotspot build broken on linux-sparc after 8238281

[Kim Barrett]

> On Feb 16, 2020, at 2:58 PM, John Paul Adrian Glaubitz 
>  wrote:
> 
> Hi Kim!
> 
> On 2/15/20 10:30 PM, Kim Barrett wrote:
>> I prefer the approach using a non-ODR-used dummy to the approach of
>> casting of some random number to an address.  Non-ODR-used
>> declarations are well supported by the standard (C++03 3.2) and widely
>> used; that's the basis for the "sizeof trick" metaprogramming idiom,
>> for example.
> 
> I've now understood that approach and implemented it. Please see my patch in 
> [1].
> 
> FWIW, I have tried to put the declaration of RegistersForDebugging outside
> the X_offset() functions but that doesn't work. Hope the change is now
> okay as is :).
> 
> Adrian
> 
>> [1] http://cr.openjdk.java.net/~glaubitz/8239001/webrev.02/

Nope.  That unnecessarily pollutes the global namespace with “dummy_regs”.

The dummy declaration should be in the RegistersForDebugging class as I 
suggested earlier.
But you say that “doesn’t work”.  I thought perhaps that meant I’d made was a 
mistake in the
code snippet I’d sent, but that seems to work for me.  So what do you mean by 
“doesn’t work”?

Re: RFR: 8239001: Hotspot build broken on linux-sparc after 8238281

[John Paul Adrian Glaubitz]

Hi Kim!

On 2/15/20 10:30 PM, Kim Barrett wrote:
> I prefer the approach using a non-ODR-used dummy to the approach of
> casting of some random number to an address.  Non-ODR-used
> declarations are well supported by the standard (C++03 3.2) and widely
> used; that's the basis for the "sizeof trick" metaprogramming idiom,
> for example.

I've now understood that approach and implemented it. Please see my patch in 
[1].

FWIW, I have tried to put the declaration of RegistersForDebugging outside
the X_offset() functions but that doesn't work. Hope the change is now
okay as is :).

Adrian

> [1] http://cr.openjdk.java.net/~glaubitz/8239001/webrev.02/

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: RFR: 8239001: Hotspot build broken on linux-sparc after 8238281

[John Paul Adrian Glaubitz]

Hi Andrew!

On 2/16/20 12:07 PM, Andrew Haley wrote:
> On 2/15/20 6:33 PM, John Paul Adrian Glaubitz wrote:
>>
>> Here is another approach for fixing 8238281 [1]. Please review.
>>
>>> [1] http://cr.openjdk.java.net/~glaubitz/8239001/webrev.01/
> 
> Can you give us a bit more explanation about why 8238281 broke linux-sparc,
> and why your proposal is valid? This doesn't look valid to me:

The problem is that i[j] is a non-const expression which is not allowed
when using offsetof(). I have included the GCC error message in the
bug report [1].

>   (((RegistersForDebugging*)16)->f[0])
> 

This basically just calculates the offset within the array f with the
offset j, hence the factor j.

I have to admit though that I haven't fully understood Kim's non-odr-used
approach yet.

Thanks,
Adrian

> [1] https://bugs.openjdk.java.net/browse/JDK-8239001

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: RFR: 8239001: Hotspot build broken on linux-sparc after 8238281

[Andrew Haley]

On 2/16/20 11:07 AM, Andrew Haley wrote:
> Can you give us a bit more explanation about why 8238281 broke linux-sparc,
> and why your proposal is valid? This doesn't look valid to me:
> 
>   (((RegistersForDebugging*)16)->f[0])

Sorry, I didn't see the other thread.

-- 
Andrew Haley  (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. 
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Re: RFR: 8239001: Hotspot build broken on linux-sparc after 8238281

[Andrew Haley]

On 2/15/20 6:33 PM, John Paul Adrian Glaubitz wrote:
> 
> Here is another approach for fixing 8238281 [1]. Please review.
> 
>> [1] http://cr.openjdk.java.net/~glaubitz/8239001/webrev.01/

Can you give us a bit more explanation about why 8238281 broke linux-sparc,
and why your proposal is valid? This doesn't look valid to me:

  (((RegistersForDebugging*)16)->f[0])

-- 
Andrew Haley  (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. 
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Re: RFR: 8239001: Hotspot build broken on linux-sparc after 8238281

[Kim Barrett]

> On Feb 15, 2020, at 1:06 PM, John Paul Adrian Glaubitz 
>  wrote:
> 
> On 2/13/20 8:35 PM, Kim Barrett wrote:
>> I don't think this is the right way to address this problem.
>> 
>> I think the JDK-8238281 change to offset_of and the associated
>> addition of -Wno-invalid-offsetof to the build configuration was a
>> mistake, and should be reverted.  Those changes seem unrelated to the
>> purpose of JDK-8238281, which was to raise the minimum acceptable gcc
>> version.
> 
> Alternatively, this works as well:
> 
> glaubitz@gcc202:~/jdk-hg$ hg diff
> diff -r 274a0bcce99d src/hotspot/cpu/sparc/macroAssembler_sparc.hpp
> --- a/src/hotspot/cpu/sparc/macroAssembler_sparc.hppSat Feb 15 17:35:57 
> 2020 +0800
> +++ b/src/hotspot/cpu/sparc/macroAssembler_sparc.hppSat Feb 15 21:05:10 
> 2020 +0300
> @@ -485,12 +485,12 @@
> 
>   void print(outputStream* s);
> 
> -  static int i_offset(int j) { return offset_of(RegistersForDebugging, 
> i[j]); }
> -  static int l_offset(int j) { return offset_of(RegistersForDebugging, 
> l[j]); }
> -  static int o_offset(int j) { return offset_of(RegistersForDebugging, 
> o[j]); }
> -  static int g_offset(int j) { return offset_of(RegistersForDebugging, 
> g[j]); }
> -  static int f_offset(int j) { return offset_of(RegistersForDebugging, 
> f[j]); }
> -  static int d_offset(int j) { return offset_of(RegistersForDebugging, d[j / 
> 2]); }
> +  static int i_offset(int j) { return offset_of(RegistersForDebugging, i) + 
> j * sizeof(((RegistersForDebugging*)16)->i[0]); }
> +  static int l_offset(int j) { return offset_of(RegistersForDebugging, l) + 
> j * sizeof(((RegistersForDebugging*)16)->l[0]); }
> +  static int o_offset(int j) { return offset_of(RegistersForDebugging, o) + 
> j * sizeof(((RegistersForDebugging*)16)->o[0]); }
> +  static int g_offset(int j) { return offset_of(RegistersForDebugging, g) + 
> j * sizeof(((RegistersForDebugging*)16)->g[0]); }
> +  static int f_offset(int j) { return offset_of(RegistersForDebugging, f) + 
> j * sizeof(((RegistersForDebugging*)16)->f[0]); }
> +  static int d_offset(int j) { return offset_of(RegistersForDebugging, d) + 
> (j / 2) * sizeof(((RegistersForDebugging*)16)->d[0]); }
> 
>   // gen asm code to save regs
>   static void save_registers(MacroAssembler* a);
> 
> What about this approach?
> 
> Alternatively, with C++11, this should work as well:
> 
> #include 
> 
> static int i_offset(int j) { return offset_of(RegistersForDebugging, i) + j * 
> sizeof(std::declval().i[0]); }
> 
> Comments?

I prefer the approach using a non-ODR-used dummy to the approach of
casting of some random number to an address.  Non-ODR-used
declarations are well supported by the standard (C++03 3.2) and widely
used; that's the basis for the "sizeof trick" metaprogramming idiom,
for example.  Using std::declval is morally equivalent, but we haven't
turned on C++11 yet.

Re: RFR: 8239001: Hotspot build broken on linux-sparc after 8238281

[John Paul Adrian Glaubitz]

On 2/13/20 8:35 PM, Kim Barrett wrote:
> I don't think this is the right way to address this problem.
> 
> I think the JDK-8238281 change to offset_of and the associated
> addition of -Wno-invalid-offsetof to the build configuration was a
> mistake, and should be reverted.  Those changes seem unrelated to the
> purpose of JDK-8238281, which was to raise the minimum acceptable gcc
> version.

Alternatively, this works as well:

glaubitz@gcc202:~/jdk-hg$ hg diff
diff -r 274a0bcce99d src/hotspot/cpu/sparc/macroAssembler_sparc.hpp
--- a/src/hotspot/cpu/sparc/macroAssembler_sparc.hppSat Feb 15 17:35:57 
2020 +0800
+++ b/src/hotspot/cpu/sparc/macroAssembler_sparc.hppSat Feb 15 21:05:10 
2020 +0300
@@ -485,12 +485,12 @@
 
   void print(outputStream* s);
 
-  static int i_offset(int j) { return offset_of(RegistersForDebugging, i[j]); }
-  static int l_offset(int j) { return offset_of(RegistersForDebugging, l[j]); }
-  static int o_offset(int j) { return offset_of(RegistersForDebugging, o[j]); }
-  static int g_offset(int j) { return offset_of(RegistersForDebugging, g[j]); }
-  static int f_offset(int j) { return offset_of(RegistersForDebugging, f[j]); }
-  static int d_offset(int j) { return offset_of(RegistersForDebugging, d[j / 
2]); }
+  static int i_offset(int j) { return offset_of(RegistersForDebugging, i) + j 
* sizeof(((RegistersForDebugging*)16)->i[0]); }
+  static int l_offset(int j) { return offset_of(RegistersForDebugging, l) + j 
* sizeof(((RegistersForDebugging*)16)->l[0]); }
+  static int o_offset(int j) { return offset_of(RegistersForDebugging, o) + j 
* sizeof(((RegistersForDebugging*)16)->o[0]); }
+  static int g_offset(int j) { return offset_of(RegistersForDebugging, g) + j 
* sizeof(((RegistersForDebugging*)16)->g[0]); }
+  static int f_offset(int j) { return offset_of(RegistersForDebugging, f) + j 
* sizeof(((RegistersForDebugging*)16)->f[0]); }
+  static int d_offset(int j) { return offset_of(RegistersForDebugging, d) + (j 
/ 2) * sizeof(((RegistersForDebugging*)16)->d[0]); }
 
   // gen asm code to save regs
   static void save_registers(MacroAssembler* a);

What about this approach?

Alternatively, with C++11, this should work as well:

#include 

static int i_offset(int j) { return offset_of(RegistersForDebugging, i) + j * 
sizeof(std::declval().i[0]); }

Comments?

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: RFR: 8239001: Hotspot build broken on linux-sparc after 8238281

[Kim Barrett]

> On Feb 13, 2020, at 5:21 PM, Magnus Ihse Bursie 
>  wrote:
> 
> I searched the code base for mentions of old gcc workarounds, and tried to 
> remedy them now that they were not needed. 
> 
> If it turns out that they still were needed, by all means, revert that part 
> of the patch. 

I think the offset_of definition in question was not an "old gcc
workaround".

offsetof with a non-POD type is undefined behavior in C++98.  C++11
relaxed the restriction to allow standard-layout types.  C++17 changed
offsetof with a non-standard-layout type to be "conditionally
supported", which is more or less effectively UB for portable code.

Contrary to what was said in the RFR for JDK-8238281, we should not be
considering eliminating offset_of and just using offsetof directly.
offset_of is the HotSpot portability shim to let us knowingly do
something that is potentially problematic but we claim we know what
we're doing.  The fact that gcc warns about problematic offsetof uses
(unless -Wno-invalid-offsetof) is a good thing, because it helps keep
us honest about using that portability shim.  (This is similar in
concept to JDK-8214976.)  (There are a couple of direct uses of
offsetof in os_perf_solaris.cpp.)

Then there's the separate issue that the failing code in the
RegistersForDebugging class is using offset_of / offsetof in an
invalid way, since the result is supposed to be a constant expression
and that's impossible because these uses involve the non-constant "j"
argument.

I think that can be fixed by something like

/* Add this in RegistersForDebugging */
private:
  static const RegistersForDebugging& _dummy; // not ODR-used so not defined

/* Change offset functions based on this model. */
static int i_offset(int j) {
  return offset_of(RegistersForDebugging, i) + j * (sizeof _dummy.i[0]);
}

Of course, reverting to the gcc implementation of offset_of also
"fixes" this problem, because that version of offset_of happens to
"accidentally" be more lenient.

Re: RFR: 8239001: Hotspot build broken on linux-sparc after 8238281

[Magnus Ihse Bursie]

I searched the code base for mentions of old gcc workarounds, and tried to 
remedy them now that they were not needed. 

If it turns out that they still were needed, by all means, revert that part of 
the patch. 

/Magnus

13 feb. 2020 kl. 20:35 skrev Kim Barrett :

>> On Feb 13, 2020, at 7:26 AM, John Paul Adrian Glaubitz 
>>  wrote:
>> 
>> Hello!
>> 
>> Hotspot fails to build on linux-sparc after 8238281 due to the redefinition
>> of offset_of() in src/hotspot/share/prims/jvm.cpp [1]:
>> 
>> /home/glaubitz/jdk/src/hotspot/cpu/sparc/macroAssembler_sparc.hpp: In static 
>> member function 'static int RegistersForDebugging::i_offset(int)':
>> /home/glaubitz/jdk/src/hotspot/cpu/sparc/macroAssembler_sparc.hpp:488:74: 
>> error: 'j' cannot appear in a constant-expression
>> 488 | static int i_offset(int j) { return offset_of(RegistersForDebugging, 
>> i[j]); }
>> | ^ 
>> 
>> Since offsetof() can only be used with constant expressions, I have reused
>> the old definition of offset_of() in 
>> src/hotspot/cpu/sparc/macroAssembler_sparc.hpp
>> to fix the issue.
>> 
>> I couldn't come up with a more elegant solution, but I'm open for 
>> suggestions.
>> 
>> Please review my change in [2].
>> 
>> Thanks,
>> Adrian
>> 
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8239001
>>> [2] http://cr.openjdk.java.net/~glaubitz/8239001/webrev.00/
> 
> I don't think this is the right way to address this problem.
> 
> I think the JDK-8238281 change to offset_of and the associated
> addition of -Wno-invalid-offsetof to the build configuration was a
> mistake, and should be reverted.  Those changes seem unrelated to the
> purpose of JDK-8238281, which was to raise the minimum acceptable gcc
> version.
> 

Re: RFR: 8239001: Hotspot build broken on linux-sparc after 8238281

[Kim Barrett]

> On Feb 13, 2020, at 7:26 AM, John Paul Adrian Glaubitz 
>  wrote:
> 
> Hello!
> 
> Hotspot fails to build on linux-sparc after 8238281 due to the redefinition
> of offset_of() in src/hotspot/share/prims/jvm.cpp [1]:
> 
> /home/glaubitz/jdk/src/hotspot/cpu/sparc/macroAssembler_sparc.hpp: In static 
> member function 'static int RegistersForDebugging::i_offset(int)':
> /home/glaubitz/jdk/src/hotspot/cpu/sparc/macroAssembler_sparc.hpp:488:74: 
> error: 'j' cannot appear in a constant-expression
>  488 | static int i_offset(int j) { return offset_of(RegistersForDebugging, 
> i[j]); }
>  | ^ 
> 
> Since offsetof() can only be used with constant expressions, I have reused
> the old definition of offset_of() in 
> src/hotspot/cpu/sparc/macroAssembler_sparc.hpp
> to fix the issue.
> 
> I couldn't come up with a more elegant solution, but I'm open for suggestions.
> 
> Please review my change in [2].
> 
> Thanks,
> Adrian
> 
>> [1] https://bugs.openjdk.java.net/browse/JDK-8239001
>> [2] http://cr.openjdk.java.net/~glaubitz/8239001/webrev.00/

I don't think this is the right way to address this problem.

I think the JDK-8238281 change to offset_of and the associated
addition of -Wno-invalid-offsetof to the build configuration was a
mistake, and should be reverted.  Those changes seem unrelated to the
purpose of JDK-8238281, which was to raise the minimum acceptable gcc
version.