[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-dev
Subject:    RE: RFR(S) 8239449: [TESTBUG] test/hotspot/jtreg/runtime/TLS/TestTLS.java: skip test if glibc too ol
From:       "Reingruber, Richard" <richard.reingruber () sap ! com>
Date:       2020-02-24 13:38:57
Message-ID: AM0PR0202MB3331FB58A032F0B4B07AD5C79BEC0 () AM0PR0202MB3331 ! eurprd02 ! prod ! outlook ! com
[Download RAW message or body]

Hi Jiangli,

Thanks for reviewing and providing feedback. I just pushed the change.

Best regards,
Richard.

-----Original Message-----
From: Jiangli Zhou <jianglizhou@google.com> 
Sent: Freitag, 21. Februar 2020 19:26
To: Reingruber, Richard <richard.reingruber@sap.com>
Cc: hotspot-dev@openjdk.java.net; hotspot-runtime-dev@openjdk.java.net
Subject: Re: RFR(S) 8239449: [TESTBUG] test/hotspot/jtreg/runtime/TLS/TestTLS.java: \
skip test if glibc too old for AdjustStackSizeForTLS

Hi Richard,

The latest test update looks fine to me. Thank you for digging deeper into this.

Best regards,

Jiangli

On Fri, Feb 21, 2020 at 3:25 AM Reingruber, Richard
<richard.reingruber@sap.com> wrote:
> 
> Hi Jiangli,
> 
> > > Actually I would prefer not to skip the test then. I'd like to verify if large \
> > > data structures can be placed in TLS without stack size adjustment or if there \
> > > is some kind of regression. That's the point of regression tests after all.
> > > 
> 
> > That's a very valid point. I thought about that as well yesterday. My
> > thinking was that the VM and test would need to be updated to remove
> > the workaround for the TLS issue when no longer needed. Do you know if
> > the proposed check using gnu_get_libc_version() covers all cases? I
> 
> Other C libraries (e.g. MUSL) are not really covered, but I'm not sure if you mean \
> this by covered... 
> > came across the following archive that mentioned missing symbol in
> > eglibc-2.13 (eglibc however is already deprecated).
> 
> > https://www.mail-archive.com/lfs-dev@linuxfromscratch.org/msg18115.html
> 
> I only knew, that 2.11 does not provide __pthread_get_minstack, but then I used
> 
> https://github.com/bminor/glibc
> 
> to find out that __pthread_get_minstack was added with 2.14.90 [1]. So 2.15 should \
> be the first stable release to include __pthread_get_minstack. I'll correct this in \
> my fix. 
> Would you be ok with this version of the fix?


> 
> Thanks and best regards,
> Richard.
> 
> [1] version.h at [2]
> https://github.com/bminor/glibc/blob/2c1094bd700e63a8d7f547b3f5495bedb55c0a08/version.h
>  
> [2] Repo at [3]
> https://github.com/bminor/glibc/tree/2c1094bd700e63a8d7f547b3f5495bedb55c0a08
> 
> [3] Commit that introduced __pthread_get_minstack
> https://github.com/bminor/glibc/commit/2c1094bd700e63a8d7f547b3f5495bedb55c0a08
> 
> 
> -----Original Message-----
> From: Jiangli Zhou <jianglizhou@google.com>
> Sent: Donnerstag, 20. Februar 2020 17:32
> To: Reingruber, Richard <richard.reingruber@sap.com>
> Cc: hotspot-dev@openjdk.java.net; hotspot-runtime-dev@openjdk.java.net
> Subject: Re: RFR(S) 8239449: [TESTBUG] test/hotspot/jtreg/runtime/TLS/TestTLS.java: \
> skip test if glibc too old for AdjustStackSizeForTLS 
> Hi Richard,
> 
> On Thu, Feb 20, 2020 at 1:03 AM Reingruber, Richard
> <richard.reingruber@sap.com> wrote:
> > 
> > Hi Jiangli,
> > 
> > thanks for providing feedback.
> > 
> > > David's suggestion of using dlsym seems to be more reliable. Regarding the \
> > > stability of __pthread_get_minstack in future glibc versions, Florian has added \
> > > comment to __pthread_get_minstack about external users to ensure it's not \
> > > removed until TCB is separately allocated.  Please see: \
> > > http://51.15.138.76/patch/18989/ and \
> > > https://gcc.gnu.org/bugzilla//show_bug.cgi?id=91628#c2.
> > 
> > So if I understand you and David correctly, then the test should be skipped if \
> > the lookup of __pthread_get_minstack fails.
> > 
> > Then the test would be skipped on future glibc versions where \
> > __pthread_get_minstack was removed because TLS and TCB are not placed on a \
> > threads stack anymore. 
> > Actually I would prefer not to skip the test then. I'd like to verify if large \
> > data structures can be placed in TLS without stack size adjustment or if there is \
> > some kind of regression. That's the point of regression tests after all.
> > 
> 
> That's a very valid point. I thought about that as well yesterday. My
> thinking was that the VM and test would need to be updated to remove
> the workaround for the TLS issue when no longer needed. Do you know if
> the proposed check using gnu_get_libc_version() covers all cases? I
> came across the following archive that mentioned missing symbol in
> eglibc-2.13 (eglibc however is already deprecated).
> 
> https://www.mail-archive.com/lfs-dev@linuxfromscratch.org/msg18115.html
> 
> > In other words: I wouldn't change test coverage of future glibc versions. I just \
> > want to exclude glibc versions were the tested feature cannot work.
> 
> Best regards,
> Jiangli
> > 
> > Thanks, Richard.
> > 
> > -----Original Message-----
> > From: Jiangli Zhou <jianglizhou@google.com>
> > Sent: Mittwoch, 19. Februar 2020 19:24
> > To: Reingruber, Richard <richard.reingruber@sap.com>
> > Cc: hotspot-dev@openjdk.java.net; hotspot-runtime-dev@openjdk.java.net
> > Subject: Re: RFR(S) 8239449: [TESTBUG] \
> > test/hotspot/jtreg/runtime/TLS/TestTLS.java: skip test if glibc too old for \
> > AdjustStackSizeForTLS 
> > Hi Richard,
> > 
> > David's suggestion of using dlsym seems to be more reliable. Regarding
> > the stability of __pthread_get_minstack in future glibc versions,
> > Florian has added comment to __pthread_get_minstack about external
> > users to ensure it's not removed until TCB is separately allocated.
> > Please see: http://51.15.138.76/patch/18989/ and
> > https://gcc.gnu.org/bugzilla//show_bug.cgi?id=91628#c2.
> > 
> > Best regards,
> > Jiangli
> > 
> > 
> > 
> > 
> > On Wed, Feb 19, 2020 at 2:44 AM Reingruber, Richard
> > <richard.reingruber@sap.com> wrote:
> > > 
> > > Hi,
> > > 
> > > please review this small test fix
> > > 
> > > Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8239449/webrev.0/
> > > Bug:    https://bugs.openjdk.java.net/browse/JDK-8239449
> > > 
> > > On linux systems with older glibc versions (e.g. 2.11) the option \
> > > -XX:+AdjustStackSizeForTLS has no effect, because the private function \
> > > __pthread_get_minstack is not provided, and the test fails. With the fix the \
> > > test is skipped instead. 
> > > Thanks, Richard.


[prev in list] [next in list] [prev in thread] [next in thread]