Quantcast

[10] RFR for 'JDK-8177958: Possible uninitialized char* in vm_version_solaris_sparc.cpp'

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[10] RFR for 'JDK-8177958: Possible uninitialized char* in vm_version_solaris_sparc.cpp'

Shafi Ahmad
Hi,

Please review the one line trivial  change for the fix of bug 'JDK-8177958: Possible uninitialized char* in vm_version_solaris_sparc.cpp'

Summary:
I have initialized the uninitialized variable 'imp' with "unknown" as CPU implementation .

jdk10 bug: https://bugs.openjdk.java.net/browse/JDK-8177958
webrev link: http://cr.openjdk.java.net/~shshahma/8177958/webrev.00/

Testing: run jprt

Regards,
Shafi
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR for 'JDK-8177958: Possible uninitialized char* in vm_version_solaris_sparc.cpp'

Aleksey Shipilev-4
On 04/26/2017 10:23 AM, Shafi Ahmad wrote:
> jdk10 bug: https://bugs.openjdk.java.net/browse/JDK-8177958
> webrev link: http://cr.openjdk.java.net/~shshahma/8177958/webrev.00/

Obviously correct. Looks good.

-Aleksey


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: [10] RFR for 'JDK-8177958: Possible uninitialized char* in vm_version_solaris_sparc.cpp'

Shafi Ahmad
Hi Aleksey,

Thank you for the review.

Regards,
Shafi

> -----Original Message-----
> From: Aleksey Shipilev [mailto:[hidden email]]
> Sent: Wednesday, April 26, 2017 1:58 PM
> To: Shafi Ahmad <[hidden email]>; hotspot-
> [hidden email]
> Subject: Re: [10] RFR for 'JDK-8177958: Possible uninitialized char* in
> vm_version_solaris_sparc.cpp'
>
> On 04/26/2017 10:23 AM, Shafi Ahmad wrote:
> > jdk10 bug: https://bugs.openjdk.java.net/browse/JDK-8177958
> > webrev link: http://cr.openjdk.java.net/~shshahma/8177958/webrev.00/
>
> Obviously correct. Looks good.
>
> -Aleksey
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR for 'JDK-8177958: Possible uninitialized char* in vm_version_solaris_sparc.cpp'

KEVIN WALLS
In reply to this post by Shafi Ahmad

Thanks Shafi, looks good to me. 8-)

--
Kevin


On 26/04/2017 09:23, Shafi Ahmad wrote:

> Hi,
>
> Please review the one line trivial  change for the fix of bug 'JDK-8177958: Possible uninitialized char* in vm_version_solaris_sparc.cpp'
>
> Summary:
> I have initialized the uninitialized variable 'imp' with "unknown" as CPU implementation .
>
> jdk10 bug: https://bugs.openjdk.java.net/browse/JDK-8177958
> webrev link: http://cr.openjdk.java.net/~shshahma/8177958/webrev.00/
>
> Testing: run jprt
>
> Regards,
> Shafi

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR for 'JDK-8177958: Possible uninitialized char* in vm_version_solaris_sparc.cpp'

David Holmes
In reply to this post by Shafi Ahmad
Hi Shafi,

On 26/04/2017 6:23 PM, Shafi Ahmad wrote:
> Hi,
>
> Please review the one line trivial  change for the fix of bug 'JDK-8177958: Possible uninitialized char* in vm_version_solaris_sparc.cpp'
>
> Summary:
> I have initialized the uninitialized variable 'imp' with "unknown" as CPU implementation .

That fixes the immediate problem - thanks.

However the basic problem that kstat failures go unreported/logged
persists. If the assert is triggered all we will see is "Unknown CPU
implementation unknown" - which is not very enlightening. Perhaps a
future enhancement ...

David

> jdk10 bug: https://bugs.openjdk.java.net/browse/JDK-8177958
> webrev link: http://cr.openjdk.java.net/~shshahma/8177958/webrev.00/
>
> Testing: run jprt
>
> Regards,
> Shafi
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: [10] RFR for 'JDK-8177958: Possible uninitialized char* in vm_version_solaris_sparc.cpp'

Shafi Ahmad
Thank you David and Kevin.

Other possible change may be like -

diff -r 762465099d93 src/os_cpu/solaris_sparc/vm/vm_version_solaris_sparc.cpp
--- a/src/os_cpu/solaris_sparc/vm/vm_version_solaris_sparc.cpp Sat Apr 22 00:21:28 2017 +0000
+++ b/src/os_cpu/solaris_sparc/vm/vm_version_solaris_sparc.cpp Sun Apr 23 23:49:42 2017 -0700
@@ -404,7 +404,7 @@
   // is available to us as well
   Sysinfo cpu_info(SI_CPUBRAND);
   bool use_solaris_12_api = cpu_info.valid();
-  const char* impl;
+  const char* impl = "Unknown";
   int impl_m = 0;
   if (use_solaris_12_api) {
     impl = cpu_info.value();
@@ -431,7 +431,7 @@
       kstat_close(kc);
     }
   }
-  assert(impl_m != 0, "Unknown CPU implementation %s", impl);
+  assert(impl_m != 0, "%s CPU implementation", impl);
   features |= impl_m;
 
   bool is_sun4v = (features & sun4v_m) != 0;

After the above change we will lose the old message format.

Regards,
Shafi

> -----Original Message-----
> From: David Holmes
> Sent: Wednesday, April 26, 2017 6:04 PM
> To: Shafi Ahmad <[hidden email]>; hotspot-
> [hidden email]
> Subject: Re: [10] RFR for 'JDK-8177958: Possible uninitialized char* in
> vm_version_solaris_sparc.cpp'
>
> Hi Shafi,
>
> On 26/04/2017 6:23 PM, Shafi Ahmad wrote:
> > Hi,
> >
> > Please review the one line trivial  change for the fix of bug 'JDK-8177958:
> Possible uninitialized char* in vm_version_solaris_sparc.cpp'
> >
> > Summary:
> > I have initialized the uninitialized variable 'imp' with "unknown" as CPU
> implementation .
>
> That fixes the immediate problem - thanks.
>
> However the basic problem that kstat failures go unreported/logged persists.
> If the assert is triggered all we will see is "Unknown CPU implementation
> unknown" - which is not very enlightening. Perhaps a future enhancement ...
>
> David
>
> > jdk10 bug: https://bugs.openjdk.java.net/browse/JDK-8177958
> > webrev link: http://cr.openjdk.java.net/~shshahma/8177958/webrev.00/
> >
> > Testing: run jprt
> >
> > Regards,
> > Shafi
> >
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR for 'JDK-8177958: Possible uninitialized char* in vm_version_solaris_sparc.cpp'

David Holmes
On 26/04/2017 10:44 PM, Shafi Ahmad wrote:

> Thank you David and Kevin.
>
> Other possible change may be like -
>
> diff -r 762465099d93 src/os_cpu/solaris_sparc/vm/vm_version_solaris_sparc.cpp
> --- a/src/os_cpu/solaris_sparc/vm/vm_version_solaris_sparc.cpp Sat Apr 22 00:21:28 2017 +0000
> +++ b/src/os_cpu/solaris_sparc/vm/vm_version_solaris_sparc.cpp Sun Apr 23 23:49:42 2017 -0700
> @@ -404,7 +404,7 @@
>    // is available to us as well
>    Sysinfo cpu_info(SI_CPUBRAND);
>    bool use_solaris_12_api = cpu_info.valid();
> -  const char* impl;
> +  const char* impl = "Unknown";
>    int impl_m = 0;
>    if (use_solaris_12_api) {
>      impl = cpu_info.value();
> @@ -431,7 +431,7 @@
>        kstat_close(kc);
>      }
>    }
> -  assert(impl_m != 0, "Unknown CPU implementation %s", impl);
> +  assert(impl_m != 0, "%s CPU implementation", impl);

I would change to:

assert(impl_m != 0, "Unrecognized CPU implementation: %s", impl);

That assertion is checking the recognition of the impl string, not
whether we obtained an impl string in the first place.

Thanks,
David

>    features |= impl_m;
>
>    bool is_sun4v = (features & sun4v_m) != 0;
>
> After the above change we will lose the old message format.
>
> Regards,
> Shafi
>
>> -----Original Message-----
>> From: David Holmes
>> Sent: Wednesday, April 26, 2017 6:04 PM
>> To: Shafi Ahmad <[hidden email]>; hotspot-
>> [hidden email]
>> Subject: Re: [10] RFR for 'JDK-8177958: Possible uninitialized char* in
>> vm_version_solaris_sparc.cpp'
>>
>> Hi Shafi,
>>
>> On 26/04/2017 6:23 PM, Shafi Ahmad wrote:
>>> Hi,
>>>
>>> Please review the one line trivial  change for the fix of bug 'JDK-8177958:
>> Possible uninitialized char* in vm_version_solaris_sparc.cpp'
>>>
>>> Summary:
>>> I have initialized the uninitialized variable 'imp' with "unknown" as CPU
>> implementation .
>>
>> That fixes the immediate problem - thanks.
>>
>> However the basic problem that kstat failures go unreported/logged persists.
>> If the assert is triggered all we will see is "Unknown CPU implementation
>> unknown" - which is not very enlightening. Perhaps a future enhancement ...
>>
>> David
>>
>>> jdk10 bug: https://bugs.openjdk.java.net/browse/JDK-8177958
>>> webrev link: http://cr.openjdk.java.net/~shshahma/8177958/webrev.00/
>>>
>>> Testing: run jprt
>>>
>>> Regards,
>>> Shafi
>>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: [10] RFR for 'JDK-8177958: Possible uninitialized char* in vm_version_solaris_sparc.cpp'

Shafi Ahmad
Thank you David.

Regards,
Shafi

> -----Original Message-----
> From: David Holmes
> Sent: Thursday, April 27, 2017 2:20 AM
> To: Shafi Ahmad <[hidden email]>; hotspot-
> [hidden email]
> Subject: Re: [10] RFR for 'JDK-8177958: Possible uninitialized char* in
> vm_version_solaris_sparc.cpp'
>
> On 26/04/2017 10:44 PM, Shafi Ahmad wrote:
> > Thank you David and Kevin.
> >
> > Other possible change may be like -
> >
> > diff -r 762465099d93
> src/os_cpu/solaris_sparc/vm/vm_version_solaris_sparc.cpp
> > --- a/src/os_cpu/solaris_sparc/vm/vm_version_solaris_sparc.cpp Sat
> Apr 22 00:21:28 2017 +0000
> > +++ b/src/os_cpu/solaris_sparc/vm/vm_version_solaris_sparc.cpp Sun
> Apr 23 23:49:42 2017 -0700
> > @@ -404,7 +404,7 @@
> >    // is available to us as well
> >    Sysinfo cpu_info(SI_CPUBRAND);
> >    bool use_solaris_12_api = cpu_info.valid();
> > -  const char* impl;
> > +  const char* impl = "Unknown";
> >    int impl_m = 0;
> >    if (use_solaris_12_api) {
> >      impl = cpu_info.value();
> > @@ -431,7 +431,7 @@
> >        kstat_close(kc);
> >      }
> >    }
> > -  assert(impl_m != 0, "Unknown CPU implementation %s", impl);
> > +  assert(impl_m != 0, "%s CPU implementation", impl);
>
> I would change to:
>
> assert(impl_m != 0, "Unrecognized CPU implementation: %s", impl);
>
> That assertion is checking the recognition of the impl string, not whether we
> obtained an impl string in the first place.
>
> Thanks,
> David
>
> >    features |= impl_m;
> >
> >    bool is_sun4v = (features & sun4v_m) != 0;
> >
> > After the above change we will lose the old message format.
> >
> > Regards,
> > Shafi
> >
> >> -----Original Message-----
> >> From: David Holmes
> >> Sent: Wednesday, April 26, 2017 6:04 PM
> >> To: Shafi Ahmad <[hidden email]>; hotspot-
> >> [hidden email]
> >> Subject: Re: [10] RFR for 'JDK-8177958: Possible uninitialized char*
> >> in vm_version_solaris_sparc.cpp'
> >>
> >> Hi Shafi,
> >>
> >> On 26/04/2017 6:23 PM, Shafi Ahmad wrote:
> >>> Hi,
> >>>
> >>> Please review the one line trivial  change for the fix of bug 'JDK-8177958:
> >> Possible uninitialized char* in vm_version_solaris_sparc.cpp'
> >>>
> >>> Summary:
> >>> I have initialized the uninitialized variable 'imp' with "unknown"
> >>> as CPU
> >> implementation .
> >>
> >> That fixes the immediate problem - thanks.
> >>
> >> However the basic problem that kstat failures go unreported/logged
> persists.
> >> If the assert is triggered all we will see is "Unknown CPU
> >> implementation unknown" - which is not very enlightening. Perhaps a
> future enhancement ...
> >>
> >> David
> >>
> >>> jdk10 bug: https://bugs.openjdk.java.net/browse/JDK-8177958
> >>> webrev link:
> http://cr.openjdk.java.net/~shshahma/8177958/webrev.00/
> >>>
> >>> Testing: run jprt
> >>>
> >>> Regards,
> >>> Shafi
> >>>
Loading...