RFR: 8263718: unused-result warning happens at os_linux.cpp

classic Classic list List threaded Threaded
53 messages Options
123
Reply | Threaded
Open this post in threaded view
|

RFR: 8263718: unused-result warning happens at os_linux.cpp

Yasumasa Suenaga-7
I tried to build OpenJDK with g++-10.2.1_pre1-r3 on Alpine Linux 3.13.2, but I saw following warning:


  668 | alloca(((pid ^ counter++) & 7) * 128);
      | ^
cc1plus: all warnings being treated as errors

-------------

Commit messages:
 - 8263718: unused-result warning happens at os_linux.cpp

Changes: https://git.openjdk.java.net/jdk/pull/3042/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3042&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8263718
  Stats: 11 lines in 3 files changed: 9 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3042.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3042/head:pull/3042

PR: https://git.openjdk.java.net/jdk/pull/3042
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263718: unused-result warning happens at os_linux.cpp

David Holmes
Hi Yasumasa,

On 17/03/2021 6:37 pm, Yasumasa Suenaga wrote:
> I tried to build OpenJDK with g++-10.2.1_pre1-r3 on Alpine Linux 3.13.2, but I saw following warning:
>
>
>    668 | alloca(((pid ^ counter++) & 7) * 128);
>        | ^
> cc1plus: all warnings being treated as errors

First I have to wonder whether that alloca actually serves any useful
purpose in this day and age? I wonder what was used to measure the
performance difference... I'll see what I can find out.

But second, will doing:

void* padding = alloca(...);

not suffice to suppress the warning, or will the compiler then complain
about "padding" being unused?

The pragma no doubt works, but is a bit ugly. :)

Thanks,
David

> -------------
>
> Commit messages:
>   - 8263718: unused-result warning happens at os_linux.cpp
>
> Changes: https://git.openjdk.java.net/jdk/pull/3042/files
>   Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3042&range=00
>    Issue: https://bugs.openjdk.java.net/browse/JDK-8263718
>    Stats: 11 lines in 3 files changed: 9 ins; 0 del; 2 mod
>    Patch: https://git.openjdk.java.net/jdk/pull/3042.diff
>    Fetch: git fetch https://git.openjdk.java.net/jdk pull/3042/head:pull/3042
>
> PR: https://git.openjdk.java.net/jdk/pull/3042
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263718: unused-result warning happens at os_linux.cpp

Yasumasa Suenaga-7
In reply to this post by Yasumasa Suenaga-7
On Wed, 17 Mar 2021 06:31:28 GMT, Yasumasa Suenaga <[hidden email]> wrote:

> I tried to build OpenJDK with g++-10.2.1_pre1-r3 on Alpine Linux 3.13.2, but I saw following warning:
>
>
>   668 | alloca(((pid ^ counter++) & 7) * 128);
>       | ^
> cc1plus: all warnings being treated as errors

> First I have to wonder whether that alloca actually serves any useful
> purpose in this day and age? I wonder what was used to measure the
> performance difference... I'll see what I can find out.

I also thought we can remove it, but I could not find any reason to do so.

> But second, will doing:
>
> void* padding = alloca(...);
>
> not suffice to suppress the warning, or will the compiler then complain
> about "padding" being unused?
>
> The pragma no doubt works, but is a bit ugly. :)

Yes, the warning was gone with `void* padding =`, but I don't want to do so because GCC has [-Wunused-variable](https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html). GCC might report the warning in future.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3042
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263718: unused-result warning happens at os_linux.cpp

Stefan Karlsson-3
On Wed, 17 Mar 2021 09:26:33 GMT, Yasumasa Suenaga <[hidden email]> wrote:

>> I tried to build OpenJDK with g++-10.2.1_pre1-r3 on Alpine Linux 3.13.2, but I saw following warning:
>>
>>
>>   668 | alloca(((pid ^ counter++) & 7) * 128);
>>       | ^
>> cc1plus: all warnings being treated as errors
>
>> First I have to wonder whether that alloca actually serves any useful
>> purpose in this day and age? I wonder what was used to measure the
>> performance difference... I'll see what I can find out.
>
> I also thought we can remove it, but I could not find any reason to do so.
>
>> But second, will doing:
>>
>> void* padding = alloca(...);
>>
>> not suffice to suppress the warning, or will the compiler then complain
>> about "padding" being unused?
>>
>> The pragma no doubt works, but is a bit ugly. :)
>
> Yes, the warning was gone with `void* padding =`, but I don't want to do so because GCC has [-Wunused-variable](https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html). GCC might report the warning in future.

Isn't the standard way to silence these kind of problem to cast the result to (void):
(void)alloca(((pid ^ counter++) & 7) * 128);

-------------

PR: https://git.openjdk.java.net/jdk/pull/3042
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263718: unused-result warning happens at os_linux.cpp

Yasumasa Suenaga-7
On Wed, 17 Mar 2021 10:18:25 GMT, Stefan Karlsson <[hidden email]> wrote:

> Isn't the standard way to silence these kind of problem to cast the result to (void):
>
> ```
> (void)alloca(((pid ^ counter++) & 7) * 128);
> ```

I tried it at first, but I still saw the warning.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3042
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263718: unused-result warning happens at os_linux.cpp

Stefan Karlsson-3
On Wed, 17 Mar 2021 10:28:56 GMT, Yasumasa Suenaga <[hidden email]> wrote:

>> Isn't the standard way to silence these kind of problem to cast the result to (void):
>> (void)alloca(((pid ^ counter++) & 7) * 128);
>
>> Isn't the standard way to silence these kind of problem to cast the result to (void):
>>
>> ```
>> (void)alloca(((pid ^ counter++) & 7) * 128);
>> ```
>
> I tried it at first, but I still saw the warning.

I see. I found this page:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425

-------------

PR: https://git.openjdk.java.net/jdk/pull/3042
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263718: unused-result warning happens at os_linux.cpp

Yasumasa Suenaga-7
On Wed, 17 Mar 2021 13:06:46 GMT, Stefan Karlsson <[hidden email]> wrote:

>>> Isn't the standard way to silence these kind of problem to cast the result to (void):
>>>
>>> ```
>>> (void)alloca(((pid ^ counter++) & 7) * 128);
>>> ```
>>
>> I tried it at first, but I still saw the warning.
>
> I see. I found this page:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425

Thanks @stefank !
Status of the Bugzilla ticket which you told is unconfirmed (and it does not seem to be updated since last year!).

I want to suppress the warning if we cannot remove `alloca()` call.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3042
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263718: unused-result warning happens at os_linux.cpp

Magnus Ihse Bursie-3
On Wed, 17 Mar 2021 13:35:36 GMT, Yasumasa Suenaga <[hidden email]> wrote:

>> I see. I found this page:
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425
>
> Thanks @stefank !
> Status of the Bugzilla ticket which you told is unconfirmed (and it does not seem to be updated since last year!).
>
> I want to suppress the warning if we cannot remove `alloca()` call.

Did you notice this workaround in the gcc bug report?

"Note that a logical negation prior to cast to void is sufficient to suppress the warning:

  int void_cast_should_not_warn() {
     (void) !foo();
     //     ^-- here
     return 0;
  }
"

-------------

PR: https://git.openjdk.java.net/jdk/pull/3042
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263718: unused-result warning happens at os_linux.cpp

Yasumasa Suenaga-7
On Wed, 17 Mar 2021 16:46:13 GMT, Magnus Ihse Bursie <[hidden email]> wrote:

> "Note that a logical negation prior to cast to void is sufficient to suppress the warning:
>
> ```
>   int void_cast_should_not_warn() {
>      (void) !foo();
>      //     ^-- here
>      return 0;
>   }
> ```
>
> "

The warning has gone with `(void)!`, but isn't is strange a bit? IMHO code changes or pragma (or compiler option) are suitable than it.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3042
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263718: unused-result warning happens at os_linux.cpp

Magnus Ihse Bursie-3
On Wed, 17 Mar 2021 23:08:31 GMT, Yasumasa Suenaga <[hidden email]> wrote:

>> Did you notice this workaround in the gcc bug report?
>>
>> "Note that a logical negation prior to cast to void is sufficient to suppress the warning:
>>
>>   int void_cast_should_not_warn() {
>>      (void) !foo();
>>      //     ^-- here
>>      return 0;
>>   }
>> "
>
>> "Note that a logical negation prior to cast to void is sufficient to suppress the warning:
>>
>> ```
>>   int void_cast_should_not_warn() {
>>      (void) !foo();
>>      //     ^-- here
>>      return 0;
>>   }
>> ```
>>
>> "
>
> The warning has gone with `(void)!`, but isn't is strange a bit? IMHO code changes or pragma (or compiler option) are suitable than it.

I can't really comment on which is better -- it's up to you hotspot developers.

Nevertheless, my personal preference is that `(void)` would have been best since it's a well-established idiom. If gcc is buggy about this, then I think I'd preferred to use this workaround with a comment `// the ! is needed to workaround gcc bug`, to keep with the idiom as much as possible. Pragmas also work, but I personally consider them ugly and a last resort. But once again, don't listen to what I'm saying :-)

-------------

PR: https://git.openjdk.java.net/jdk/pull/3042
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263718: unused-result warning happens at os_linux.cpp

Thomas Stuefe
On Thu, 18 Mar 2021 09:48:27 GMT, Magnus Ihse Bursie <[hidden email]> wrote:

>>> "Note that a logical negation prior to cast to void is sufficient to suppress the warning:
>>>
>>> ```
>>>   int void_cast_should_not_warn() {
>>>      (void) !foo();
>>>      //     ^-- here
>>>      return 0;
>>>   }
>>> ```
>>>
>>> "
>>
>> The warning has gone with `(void)!`, but isn't is strange a bit? IMHO code changes or pragma (or compiler option) are suitable than it.
>
> I can't really comment on which is better -- it's up to you hotspot developers.
>
> Nevertheless, my personal preference is that `(void)` would have been best since it's a well-established idiom. If gcc is buggy about this, then I think I'd preferred to use this workaround with a comment `// the ! is needed to workaround gcc bug`, to keep with the idiom as much as possible. Pragmas also work, but I personally consider them ugly and a last resort. But once again, don't listen to what I'm saying :-)

I agree with Magnus. BTW, we are sure that alloca() call is not simply optimized away, right? Otherwise I would assign the return value to some file static volatile holder.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3042
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263718: unused-result warning happens at os_linux.cpp

Yasumasa Suenaga-7
On Thu, 18 Mar 2021 10:03:04 GMT, Thomas Stuefe <[hidden email]> wrote:

>> I can't really comment on which is better -- it's up to you hotspot developers.
>>
>> Nevertheless, my personal preference is that `(void)` would have been best since it's a well-established idiom. If gcc is buggy about this, then I think I'd preferred to use this workaround with a comment `// the ! is needed to workaround gcc bug`, to keep with the idiom as much as possible. Pragmas also work, but I personally consider them ugly and a last resort. But once again, don't listen to what I'm saying :-)
>
> I agree with Magnus. BTW, we are sure that alloca() call is not simply optimized away, right? Otherwise I would assign the return value to some file static volatile holder.

First, I think it is the best to remove `alloca()`. According to the comment, it seems to aim to reduce cache pollution. But I wonder why this `alloca()` call resolves it because stack memory will be allocated in each threads - they should be different physical memory. This code has existed since initial load, so I cannot find JBS ticket for this. So I'm not sure we can remove it yet. (comments are welcome!)

If we decide to remain this code, we need to avoid unused-result warning from GCC. I think we can use pragma because other pragmas (format-nonliteral, format-security, stringop-truncation") are used in HotSpot. In addition, this behavior does not seem to determine to be a bug in GCC (status is UNCONFIRMED). However I will agree to use `(void)!` if it is still preferred based on them.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3042
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263718: unused-result warning happens at os_linux.cpp

David Holmes
Hi Yasumasa,

On 18/03/2021 10:29 pm, Yasumasa Suenaga wrote:
> On Thu, 18 Mar 2021 10:03:04 GMT, Thomas Stuefe <[hidden email]> wrote:
>
>>> I can't really comment on which is better -- it's up to you hotspot developers.
>>>
>>> Nevertheless, my personal preference is that `(void)` would have been best since it's a well-established idiom. If gcc is buggy about this, then I think I'd preferred to use this workaround with a comment `// the ! is needed to workaround gcc bug`, to keep with the idiom as much as possible. Pragmas also work, but I personally consider them ugly and a last resort. But once again, don't listen to what I'm saying :-)
>>
>> I agree with Magnus. BTW, we are sure that alloca() call is not simply optimized away, right? Otherwise I would assign the return value to some file static volatile holder.
>
> First, I think it is the best to remove `alloca()`. According to the comment, it seems to aim to reduce cache pollution. But I wonder why this `alloca()` call resolves it because stack memory will be allocated in each threads - they should be different physical memory. This code has existed since initial load, so I cannot find JBS ticket for this. So I'm not sure we can remove it yet. (comments are welcome!)

The alloca was added as a performance boost for hyperthreaded systems
back in 2003 for JDK 5:

"A per-thread offset was added to each thread's stack to randomize the
cachelines of hot stack frames (aka, stack coloring)."

The issue is not open, hence why you could not find it, but it says very
little beyond what I just quoted.

I'm running some of our benchmarks to see if removing it makes a
difference ... but chances are I'm not going to be running it on the
kind of machines for which it was introduced.

> If we decide to remain this code, we need to avoid unused-result warning from GCC. I think we can use pragma because other pragmas (format-nonliteral, format-security, stringop-truncation") are used in HotSpot. In addition, this behavior does not seem to determine to be a bug in GCC (status is UNCONFIRMED). However I will agree to use `(void)!` if it is still preferred based on them.

I'd reluctantly prefer to use the pragma as an official mechanism for
avoiding the warning.

BTW this is not the first time we have had this issue with gcc making a
function deprecated and casting to void not fixing it. Unfortunately I
can't remember enough of the previous case's details to actually look it
up and see what we resolved to do. :(

David
-----

> -------------
>
> PR: https://git.openjdk.java.net/jdk/pull/3042
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263718: unused-result warning happens at os_linux.cpp

Yasumasa Suenaga-7
In reply to this post by Yasumasa Suenaga-7
On Thu, 18 Mar 2021 12:27:04 GMT, Yasumasa Suenaga <[hidden email]> wrote:

>> I agree with Magnus. BTW, we are sure that alloca() call is not simply optimized away, right? Otherwise I would assign the return value to some file static volatile holder.
>
> First, I think it is the best to remove `alloca()`. According to the comment, it seems to aim to reduce cache pollution. But I wonder why this `alloca()` call resolves it because stack memory will be allocated in each threads - they should be different physical memory. This code has existed since initial load, so I cannot find JBS ticket for this. So I'm not sure we can remove it yet. (comments are welcome!)
>
> If we decide to remain this code, we need to avoid unused-result warning from GCC. I think we can use pragma because other pragmas (format-nonliteral, format-security, stringop-truncation") are used in HotSpot. In addition, this behavior does not seem to determine to be a bug in GCC (status is UNCONFIRMED). However I will agree to use `(void)!` if it is still preferred based on them.

> The alloca was added as a performance boost for hyperthreaded systems
> back in 2003 for JDK 5:
>
> "A per-thread offset was added to each thread's stack to randomize the
> cachelines of hot stack frames (aka, stack coloring)."

IIUC it relates to share DTLB between two logical processors when Hyperthreading is enabled.

> I'm running some of our benchmarks to see if removing it makes a
> difference ... but chances are I'm not going to be running it on the
> kind of machines for which it was introduced.

Thanks!

> > If we decide to remain this code, we need to avoid unused-result warning from GCC. I think we can use pragma because other pragmas (format-nonliteral, format-security, stringop-truncation") are used in HotSpot. In addition, this behavior does not seem to determine to be a bug in GCC (status is UNCONFIRMED). However I will agree to use `(void)!` if it is still preferred based on them.
>
> I'd reluctantly prefer to use the pragma as an official mechanism for
> avoiding the warning.

Ok, OpenJDK folks who discuss in this PR are not prefer to use pragma, so I will use `(void)!` if it is needed.
Let's see the result of benchmark and discuss what should we do.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3042
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263718: unused-result warning happens at os_linux.cpp

Yasumasa Suenaga-7
On Thu, 18 Mar 2021 14:40:20 GMT, Yasumasa Suenaga <[hidden email]> wrote:

> > The alloca was added as a performance boost for hyperthreaded systems
> > back in 2003 for JDK 5:
> > "A per-thread offset was added to each thread's stack to randomize the
> > cachelines of hot stack frames (aka, stack coloring)."
>
> IIUC it relates to share DTLB between two logical processors when Hyperthreading is enabled.

`alloca()` call might be less effective if ASLR is enabled. It can be configured by [randomize_va_space](https://www.kernel.org/doc/html/latest/admin-guide/sysctl/kernel.html#randomize-va-space), and I guess it is enabled in a lot of x86 systems.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3042
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263718: unused-result warning happens at os_linux.cpp

Yasumasa Suenaga-7
On Fri, 19 Mar 2021 00:02:21 GMT, Yasumasa Suenaga <[hidden email]> wrote:

>>> The alloca was added as a performance boost for hyperthreaded systems
>>> back in 2003 for JDK 5:
>>>
>>> "A per-thread offset was added to each thread's stack to randomize the
>>> cachelines of hot stack frames (aka, stack coloring)."
>>
>> IIUC it relates to share DTLB between two logical processors when Hyperthreading is enabled.
>>
>>> I'm running some of our benchmarks to see if removing it makes a
>>> difference ... but chances are I'm not going to be running it on the
>>> kind of machines for which it was introduced.
>>
>> Thanks!
>>
>>> > If we decide to remain this code, we need to avoid unused-result warning from GCC. I think we can use pragma because other pragmas (format-nonliteral, format-security, stringop-truncation") are used in HotSpot. In addition, this behavior does not seem to determine to be a bug in GCC (status is UNCONFIRMED). However I will agree to use `(void)!` if it is still preferred based on them.
>>>
>>> I'd reluctantly prefer to use the pragma as an official mechanism for
>>> avoiding the warning.
>>
>> Ok, OpenJDK folks who discuss in this PR are not prefer to use pragma, so I will use `(void)!` if it is needed.
>> Let's see the result of benchmark and discuss what should we do.
>
>> > The alloca was added as a performance boost for hyperthreaded systems
>> > back in 2003 for JDK 5:
>> > "A per-thread offset was added to each thread's stack to randomize the
>> > cachelines of hot stack frames (aka, stack coloring)."
>>
>> IIUC it relates to share DTLB between two logical processors when Hyperthreading is enabled.
>
> `alloca()` call might be less effective if ASLR is enabled. It can be configured by [randomize_va_space](https://www.kernel.org/doc/html/latest/admin-guide/sysctl/kernel.html#randomize-va-space), and I guess it is enabled in a lot of x86 systems.

> BTW this is not the first time we have had this issue with gcc making a
> function deprecated and casting to void not fixing it. Unfortunately I
> can't remember enough of the previous case's details to actually look it
> up and see what we resolved to do. :(

I found [JDK-6879689](https://bugs.openjdk.java.net/browse/JDK-6879689), and it fixed unused-return as following:

https://github.com/openjdk/jdk/blob/9b5a9b61899cf649104c0ff70e14549f64a89561/src/hotspot/share/adlc/archDesc.cpp#L1082-L1083

I prefer it rather than `(void)!`.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3042
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263718: unused-result warning happens at os_linux.cpp

Thomas Stuefe
On Fri, 19 Mar 2021 00:14:38 GMT, Yasumasa Suenaga <[hidden email]> wrote:

> > BTW this is not the first time we have had this issue with gcc making a
> > function deprecated and casting to void not fixing it. Unfortunately I
> > can't remember enough of the previous case's details to actually look it
> > up and see what we resolved to do. :(
>
> I found [JDK-6879689](https://bugs.openjdk.java.net/browse/JDK-6879689), and it fixed unused-return as following:
>
> https://github.com/openjdk/jdk/blob/9b5a9b61899cf649104c0ff70e14549f64a89561/src/hotspot/share/adlc/archDesc.cpp#L1082-L1083
>
> I prefer it rather than `(void)!`.

Does that work in release builds too?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3042
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263718: unused-result warning happens at os_linux.cpp

David Holmes
In reply to this post by Yasumasa Suenaga-7
On 19/03/2021 10:17 am, Yasumasa Suenaga wrote:

> On Fri, 19 Mar 2021 00:02:21 GMT, Yasumasa Suenaga <[hidden email]> wrote:
>
>>>> The alloca was added as a performance boost for hyperthreaded systems
>>>> back in 2003 for JDK 5:
>>>>
>>>> "A per-thread offset was added to each thread's stack to randomize the
>>>> cachelines of hot stack frames (aka, stack coloring)."
>>>
>>> IIUC it relates to share DTLB between two logical processors when Hyperthreading is enabled.
>>>
>>>> I'm running some of our benchmarks to see if removing it makes a
>>>> difference ... but chances are I'm not going to be running it on the
>>>> kind of machines for which it was introduced.
>>>
>>> Thanks!
>>>
>>>>> If we decide to remain this code, we need to avoid unused-result warning from GCC. I think we can use pragma because other pragmas (format-nonliteral, format-security, stringop-truncation") are used in HotSpot. In addition, this behavior does not seem to determine to be a bug in GCC (status is UNCONFIRMED). However I will agree to use `(void)!` if it is still preferred based on them.
>>>>
>>>> I'd reluctantly prefer to use the pragma as an official mechanism for
>>>> avoiding the warning.
>>>
>>> Ok, OpenJDK folks who discuss in this PR are not prefer to use pragma, so I will use `(void)!` if it is needed.
>>> Let's see the result of benchmark and discuss what should we do.
>>
>>>> The alloca was added as a performance boost for hyperthreaded systems
>>>> back in 2003 for JDK 5:
>>>> "A per-thread offset was added to each thread's stack to randomize the
>>>> cachelines of hot stack frames (aka, stack coloring)."
>>>
>>> IIUC it relates to share DTLB between two logical processors when Hyperthreading is enabled.
>>
>> `alloca()` call might be less effective if ASLR is enabled. It can be configured by [randomize_va_space](https://www.kernel.org/doc/html/latest/admin-guide/sysctl/kernel.html#randomize-va-space), and I guess it is enabled in a lot of x86 systems.

It relates to L1 cache index collisions primarily so only impacts
hyperthreading on Intel CPUs (it also impacted CMT on SPARC). I've been
told that ASLR may render the manual stack-coloring unnecessary (as well
as ineffective), but then we'd need to establish which platforms have
that enabled, and whether we can tell.

The benchmarking has been inconclusive - no observable differences. But
the benchmarking machines (and indeed the benchmarks) may not be
representative of the context where this optimization is needed.

It has also been raised (as it was here) whether the alloca is even left
in place by the compiler. Something I have yet to check.

>> BTW this is not the first time we have had this issue with gcc making a
>> function deprecated and casting to void not fixing it. Unfortunately I
>> can't remember enough of the previous case's details to actually look it
>> up and see what we resolved to do. :(
>
> I found [JDK-6879689](https://bugs.openjdk.java.net/browse/JDK-6879689), and it fixed unused-return as following:
>
> https://github.com/openjdk/jdk/blob/9b5a9b61899cf649104c0ff70e14549f64a89561/src/hotspot/share/adlc/archDesc.cpp#L1082-L1083
>
> I prefer it rather than `(void)!`.

But introducing a local (which was already suggested earlier) that is
unused in a product build may also trigger a warning from the compiler
and we are back where we started.

David

> -------------
>
> PR: https://git.openjdk.java.net/jdk/pull/3042
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263718: unused-result warning happens at os_linux.cpp

David Holmes
Okay so this may all be moot :)

On 19/03/2021 3:09 pm, David Holmes wrote:

> On 19/03/2021 10:17 am, Yasumasa Suenaga wrote:
>> On Fri, 19 Mar 2021 00:02:21 GMT, Yasumasa Suenaga
>> <[hidden email]> wrote:
>>
>>>>> The alloca was added as a performance boost for hyperthreaded systems
>>>>> back in 2003 for JDK 5:
>>>>>
>>>>> "A per-thread offset was added to each thread's stack to randomize the
>>>>> cachelines of hot stack frames (aka, stack coloring)."
>>>>
>>>> IIUC it relates to share DTLB between two logical processors when
>>>> Hyperthreading is enabled.
>>>>
>>>>> I'm running some of our benchmarks to see if removing it makes a
>>>>> difference ... but chances are I'm not going to be running it on the
>>>>> kind of machines for which it was introduced.
>>>>
>>>> Thanks!
>>>>
>>>>>> If we decide to remain this code, we need to avoid unused-result
>>>>>> warning from GCC. I think we can use pragma because other pragmas
>>>>>> (format-nonliteral, format-security, stringop-truncation") are
>>>>>> used in HotSpot. In addition, this behavior does not seem to
>>>>>> determine to be a bug in GCC (status is UNCONFIRMED). However I
>>>>>> will agree to use `(void)!` if it is still preferred based on them.
>>>>>
>>>>> I'd reluctantly prefer to use the pragma as an official mechanism for
>>>>> avoiding the warning.
>>>>
>>>> Ok, OpenJDK folks who discuss in this PR are not prefer to use
>>>> pragma, so I will use `(void)!` if it is needed.
>>>> Let's see the result of benchmark and discuss what should we do.
>>>
>>>>> The alloca was added as a performance boost for hyperthreaded systems
>>>>> back in 2003 for JDK 5:
>>>>> "A per-thread offset was added to each thread's stack to randomize the
>>>>> cachelines of hot stack frames (aka, stack coloring)."
>>>>
>>>> IIUC it relates to share DTLB between two logical processors when
>>>> Hyperthreading is enabled.
>>>
>>> `alloca()` call might be less effective if ASLR is enabled. It can be
>>> configured by
>>> [randomize_va_space](https://www.kernel.org/doc/html/latest/admin-guide/sysctl/kernel.html#randomize-va-space),
>>> and I guess it is enabled in a lot of x86 systems.
>
> It relates to L1 cache index collisions primarily so only impacts
> hyperthreading on Intel CPUs (it also impacted CMT on SPARC). I've been
> told that ASLR may render the manual stack-coloring unnecessary (as well
> as ineffective), but then we'd need to establish which platforms have
> that enabled, and whether we can tell.
>
> The benchmarking has been inconclusive - no observable differences. But
> the benchmarking machines (and indeed the benchmarks) may not be
> representative of the context where this optimization is needed.
>
> It has also been raised (as it was here) whether the alloca is even left
> in place by the compiler. Something I have yet to check.

        call _ZN6Thread26record_stack_base_and_sizeEv@PLT
.LVL2492:
        .loc 3 666 3 is_stmt 1 view .LVU7882
        .loc 3 667 3 view .LVU7883
.LBB6399:
.LBI6399:
        .loc 3 1359 5 view .LVU7884
.LBB6400:
        .loc 3 1360 3 view .LVU7885
        .loc 3 1360 18 is_stmt 0 view .LVU7886
        call getpid@PLT
.LVL2493:
.LBE6400:
.LBE6399:
        .loc 3 668 3 is_stmt 1 view .LVU7887
        .loc 3 670 36 is_stmt 0 view .LVU7888
        movq %r13, %rdi
        .loc 3 668 3 view .LVU7889
        addl $1, _ZZL19thread_native_entryP6ThreadE7counter(%rip)
        .loc 3 670 3 is_stmt 1 view .LVU7890
        .loc 3 670 36 is_stmt 0 view .LVU7891
        call _ZN6Thread25initialize_thread_currentEv@PLT

Appears the alloca has been elided, along with the arithmetic.

Don't know if that is true for other platforms.

David
-----

>>> BTW this is not the first time we have had this issue with gcc making a
>>> function deprecated and casting to void not fixing it. Unfortunately I
>>> can't remember enough of the previous case's details to actually look it
>>> up and see what we resolved to do. :(
>>
>> I found
>> [JDK-6879689](https://bugs.openjdk.java.net/browse/JDK-6879689), and
>> it fixed unused-return as following:
>>
>> https://github.com/openjdk/jdk/blob/9b5a9b61899cf649104c0ff70e14549f64a89561/src/hotspot/share/adlc/archDesc.cpp#L1082-L1083 
>>
>>
>> I prefer it rather than `(void)!`.
>
> But introducing a local (which was already suggested earlier) that is
> unused in a product build may also trigger a warning from the compiler
> and we are back where we started.
>
> David
>
>> -------------
>>
>> PR: https://git.openjdk.java.net/jdk/pull/3042
>>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263718: unused-result warning happens at os_linux.cpp

tstuefe
On Fri, Mar 19, 2021 at 6:49 AM David Holmes <[hidden email]>
wrote:

> Okay so this may all be moot :)
>
> On 19/03/2021 3:09 pm, David Holmes wrote:
> > On 19/03/2021 10:17 am, Yasumasa Suenaga wrote:
> >> On Fri, 19 Mar 2021 00:02:21 GMT, Yasumasa Suenaga
> >> <[hidden email]> wrote:
> >>
> >>>>> The alloca was added as a performance boost for hyperthreaded systems
> >>>>> back in 2003 for JDK 5:
> >>>>>
> >>>>> "A per-thread offset was added to each thread's stack to randomize
> the
> >>>>> cachelines of hot stack frames (aka, stack coloring)."
> >>>>
> >>>> IIUC it relates to share DTLB between two logical processors when
> >>>> Hyperthreading is enabled.
> >>>>
> >>>>> I'm running some of our benchmarks to see if removing it makes a
> >>>>> difference ... but chances are I'm not going to be running it on the
> >>>>> kind of machines for which it was introduced.
> >>>>
> >>>> Thanks!
> >>>>
> >>>>>> If we decide to remain this code, we need to avoid unused-result
> >>>>>> warning from GCC. I think we can use pragma because other pragmas
> >>>>>> (format-nonliteral, format-security, stringop-truncation") are
> >>>>>> used in HotSpot. In addition, this behavior does not seem to
> >>>>>> determine to be a bug in GCC (status is UNCONFIRMED). However I
> >>>>>> will agree to use `(void)!` if it is still preferred based on them.
> >>>>>
> >>>>> I'd reluctantly prefer to use the pragma as an official mechanism for
> >>>>> avoiding the warning.
> >>>>
> >>>> Ok, OpenJDK folks who discuss in this PR are not prefer to use
> >>>> pragma, so I will use `(void)!` if it is needed.
> >>>> Let's see the result of benchmark and discuss what should we do.
> >>>
> >>>>> The alloca was added as a performance boost for hyperthreaded systems
> >>>>> back in 2003 for JDK 5:
> >>>>> "A per-thread offset was added to each thread's stack to randomize
> the
> >>>>> cachelines of hot stack frames (aka, stack coloring)."
> >>>>
> >>>> IIUC it relates to share DTLB between two logical processors when
> >>>> Hyperthreading is enabled.
> >>>
> >>> `alloca()` call might be less effective if ASLR is enabled. It can be
> >>> configured by
> >>> [randomize_va_space](
> https://www.kernel.org/doc/html/latest/admin-guide/sysctl/kernel.html#randomize-va-space),
>
> >>> and I guess it is enabled in a lot of x86 systems.
> >
> > It relates to L1 cache index collisions primarily so only impacts
> > hyperthreading on Intel CPUs (it also impacted CMT on SPARC). I've been
> > told that ASLR may render the manual stack-coloring unnecessary (as well
> > as ineffective), but then we'd need to establish which platforms have
> > that enabled, and whether we can tell.
> >
> > The benchmarking has been inconclusive - no observable differences. But
> > the benchmarking machines (and indeed the benchmarks) may not be
> > representative of the context where this optimization is needed.
> >
> > It has also been raised (as it was here) whether the alloca is even left
> > in place by the compiler. Something I have yet to check.
>
>         call    _ZN6Thread26record_stack_base_and_sizeEv@PLT
> .LVL2492:
>         .loc 3 666 3 is_stmt 1 view .LVU7882
>         .loc 3 667 3 view .LVU7883
> .LBB6399:
> .LBI6399:
>         .loc 3 1359 5 view .LVU7884
> .LBB6400:
>         .loc 3 1360 3 view .LVU7885
>         .loc 3 1360 18 is_stmt 0 view .LVU7886
>         call    getpid@PLT
> .LVL2493:
> .LBE6400:
> .LBE6399:
>         .loc 3 668 3 is_stmt 1 view .LVU7887
>         .loc 3 670 36 is_stmt 0 view .LVU7888
>         movq    %r13, %rdi
>         .loc 3 668 3 view .LVU7889
>         addl    $1, _ZZL19thread_native_entryP6ThreadE7counter(%rip)
>         .loc 3 670 3 is_stmt 1 view .LVU7890
>         .loc 3 670 36 is_stmt 0 view .LVU7891
>         call    _ZN6Thread25initialize_thread_currentEv@PLT
>
> Appears the alloca has been elided, along with the arithmetic.
>
> Don't know if that is true for other platforms.
>
> David
> -----
>
>
Which would explain why we don't see effects in benchmarks. Question is, do
we repair this or remove it.

..Thomas


> >>> BTW this is not the first time we have had this issue with gcc making a
> >>> function deprecated and casting to void not fixing it. Unfortunately I
> >>> can't remember enough of the previous case's details to actually look
> it
> >>> up and see what we resolved to do. :(
> >>
> >> I found
> >> [JDK-6879689](https://bugs.openjdk.java.net/browse/JDK-6879689), and
> >> it fixed unused-return as following:
> >>
> >>
> https://github.com/openjdk/jdk/blob/9b5a9b61899cf649104c0ff70e14549f64a89561/src/hotspot/share/adlc/archDesc.cpp#L1082-L1083
> >>
> >>
> >> I prefer it rather than `(void)!`.
> >
> > But introducing a local (which was already suggested earlier) that is
> > unused in a product build may also trigger a warning from the compiler
> > and we are back where we started.
> >
> > David
> >
> >> -------------
> >>
> >> PR: https://git.openjdk.java.net/jdk/pull/3042
> >>
>
123