RFR(xs): 8210320: PPC64: Fix uninitialized variable in C1 LIR assembler code
Hi, May I please request reviews for this tiny change that fixes two uninitialized variables in PPC64 C1 LIR code? Bug : https://bugs.openjdk.java.net/browse/JDK-8210320 Webrev: http://cr.openjdk.java.net/~gromero/8210320/v1/ GCC 4.8 does not complain about these two uninitialized pointers ('data' and 'md') but more recent versions, like 5.4.0 and 7.3.1, complain about it: In file included from /home/gromero/hg/jdk/jdk/src/hotspot/share/c1/c1_Compilation.hpp:29:0, from /home/gromero/hg/jdk/jdk/src/hotspot/share/precompiled/precompiled.hpp:286: /home/gromero/hg/jdk/jdk/src/hotspot/share/ci/ciMethodData.hpp: In member function ‘void LIR_Assembler::emit_typecheck_helper(LIR_OpTypeCheck*, Label*, Label*, Label*)’: /home/gromero/hg/jdk/jdk/src/hotspot/share/ci/ciMethodData.hpp:595:100: warning: ‘data’ may be used uninitialized in this function [-Wmaybe-uninitialized] int byte_offset_of_slot(ciProfileData* data, ByteSize slot_offset_in_data) { return in_bytes(offset_of_slot(data, slot_offset_in_data)); } ^ /home/gromero/hg/jdk/jdk/src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp:2400:18: note: ‘data’ was declared here ciProfileData* data; ^ /home/gromero/hg/jdk/jdk/src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp:2483:78: warning: ‘md’ may be used uninitialized in this function [-Wmaybe-uninitialized] type_profile_helper(mdo, mdo_offset_bias, md, data, recv, Rtmp1, success); ^ Thank you. Best regards, Gustavo
Hi Gustavo , looks good (not a reviewer however). It might not hurt to initialize md and data as well in the same file in emit_opTypeCheck as well ( even without gcc complaints ) : void LIR_Assembler::emit_opTypeCheck(LIR_OpTypeCheck* op) { LIR_Code code = op->code(); if (code == lir_store_check) { Register value = op->object()->as_register(); Register array = op->array()->as_register(); Register k_RInfo = op->tmp1()->as_register(); Register klass_RInfo = op->tmp2()->as_register(); Register Rtmp1 = op->tmp3()->as_register(); bool should_profile = op->should_profile(); __ verify_oop(value); CodeStub* stub = op->stub(); // Check if it needs to be profiled. ciMethodData* md; ciProfileData* data; ... Best regards, Matthias
-----Original Message----- From: ppc-aix-port-dev <ppc-aix-port-dev-bounces@openjdk.java.net> On Behalf Of Gustavo Romero Sent: Dienstag, 4. September 2018 15:42 To: hotspot-compiler-dev@openjdk.java.net Cc: ppc-aix-port-dev@openjdk.java.net Subject: RFR(xs): 8210320: PPC64: Fix uninitialized variable in C1 LIR assembler code Importance: High
Hi,
May I please request reviews for this tiny change that fixes two uninitialized variables in PPC64 C1 LIR code?
Bug : https://bugs.openjdk.java.net/browse/JDK-8210320 Webrev: http://cr.openjdk.java.net/~gromero/8210320/v1/
GCC 4.8 does not complain about these two uninitialized pointers ('data' and 'md') but more recent versions, like 5.4.0 and 7.3.1, complain about it:
In file included from /home/gromero/hg/jdk/jdk/src/hotspot/share/c1/c1_Compilation.hpp:29:0, from /home/gromero/hg/jdk/jdk/src/hotspot/share/precompiled/precompiled.h pp:286: /home/gromero/hg/jdk/jdk/src/hotspot/share/ci/ciMethodData.hpp: In member function ‘void LIR_Assembler::emit_typecheck_helper(LIR_OpTypeCheck*, Label*, Label*, Label*)’: /home/gromero/hg/jdk/jdk/src/hotspot/share/ci/ciMethodData.hpp:595:10 0: warning: ‘data’ may be used uninitialized in this function [-Wmaybe- uninitialized] int byte_offset_of_slot(ciProfileData* data, ByteSize slot_offset_in_data) { return in_bytes(offset_of_slot(data, slot_offset_in_data)); } ^ /home/gromero/hg/jdk/jdk/src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cp p:2400:18: note: ‘data’ was declared here ciProfileData* data; ^ /home/gromero/hg/jdk/jdk/src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cp p:2483:78: warning: ‘md’ may be used uninitialized in this function [- Wmaybe-uninitialized] type_profile_helper(mdo, mdo_offset_bias, md, data, recv, Rtmp1, success); ^
Thank you.
Best regards, Gustavo
On 09/04/2018 03:42 PM, Gustavo Romero wrote:
May I please request reviews for this tiny change that fixes two uninitialized variables in PPC64 C1 LIR code?
Bug : https://bugs.openjdk.java.net/browse/JDK-8210320 Webrev: http://cr.openjdk.java.net/~gromero/8210320/v1/
Looks good and trivial to me. -Aleksey
Hi Matthias and Aleksey, Thanks for reviewing it. On 09/04/2018 10:49 AM, Aleksey Shipilev wrote:
On 09/04/2018 03:42 PM, Gustavo Romero wrote:
May I please request reviews for this tiny change that fixes two uninitialized variables in PPC64 C1 LIR code?
Bug : https://bugs.openjdk.java.net/browse/JDK-8210320 Webrev: http://cr.openjdk.java.net/~gromero/8210320/v1/
Looks good and trivial to me.
Aleksey, I've updated that change to include another case pointed out by Matthias: http://cr.openjdk.java.net/~gromero/8210320/v2/ I think it's still trivial as before? If so it means I can push it once I receive a second OK from you? I also think I don't need to push it first to the 'submit' repo since it's a PPC64-only change. Is that correct? Thank you. Best regards, Gustavo
Hi Gustavo,
I think it's still trivial as before? Yes.
If so it means I can push it once I receive a second OK from you?
I also think I don't need to push it first to the 'submit' repo since it's a PPC64-only change. Is that correct? That's fine (assuming you have run a local build).
Best regards, Martin -----Original Message----- From: hotspot-compiler-dev <hotspot-compiler-dev-bounces@openjdk.java.net> On Behalf Of Gustavo Romero Sent: Dienstag, 4. September 2018 16:11 To: Aleksey Shipilev <shade@redhat.com>; hotspot-compiler-dev@openjdk.java.net; Baesken, Matthias <matthias.baesken@sap.com> Cc: ppc-aix-port-dev@openjdk.java.net Subject: Re: RFR(xs): 8210320: PPC64: Fix uninitialized variable in C1 LIR assembler code Hi Matthias and Aleksey, Thanks for reviewing it. On 09/04/2018 10:49 AM, Aleksey Shipilev wrote:
On 09/04/2018 03:42 PM, Gustavo Romero wrote:
May I please request reviews for this tiny change that fixes two uninitialized variables in PPC64 C1 LIR code?
Bug : https://bugs.openjdk.java.net/browse/JDK-8210320 Webrev: http://cr.openjdk.java.net/~gromero/8210320/v1/
Looks good and trivial to me.
Aleksey, I've updated that change to include another case pointed out by Matthias: http://cr.openjdk.java.net/~gromero/8210320/v2/ I think it's still trivial as before? If so it means I can push it once I receive a second OK from you? I also think I don't need to push it first to the 'submit' repo since it's a PPC64-only change. Is that correct? Thank you. Best regards, Gustavo
On 09/04/2018 11:15 AM, Doerr, Martin wrote:
Hi Gustavo,
I think it's still trivial as before? Yes.
If so it means I can push it once I receive a second OK from you?
I also think I don't need to push it first to the 'submit' repo since it's a PPC64-only change. Is that correct? That's fine (assuming you have run a local build).
Sure :) Regards, Gustavo
Best regards, Martin
-----Original Message----- From: hotspot-compiler-dev <hotspot-compiler-dev-bounces@openjdk.java.net> On Behalf Of Gustavo Romero Sent: Dienstag, 4. September 2018 16:11 To: Aleksey Shipilev <shade@redhat.com>; hotspot-compiler-dev@openjdk.java.net; Baesken, Matthias <matthias.baesken@sap.com> Cc: ppc-aix-port-dev@openjdk.java.net Subject: Re: RFR(xs): 8210320: PPC64: Fix uninitialized variable in C1 LIR assembler code
Hi Matthias and Aleksey,
Thanks for reviewing it.
On 09/04/2018 10:49 AM, Aleksey Shipilev wrote:
On 09/04/2018 03:42 PM, Gustavo Romero wrote:
May I please request reviews for this tiny change that fixes two uninitialized variables in PPC64 C1 LIR code?
Bug : https://bugs.openjdk.java.net/browse/JDK-8210320 Webrev: http://cr.openjdk.java.net/~gromero/8210320/v1/
Looks good and trivial to me.
Aleksey, I've updated that change to include another case pointed out by Matthias:
http://cr.openjdk.java.net/~gromero/8210320/v2/
I think it's still trivial as before?
If so it means I can push it once I receive a second OK from you?
I also think I don't need to push it first to the 'submit' repo since it's a PPC64-only change. Is that correct?
Thank you.
Best regards, Gustavo
On 09/04/2018 04:11 PM, Gustavo Romero wrote:
http://cr.openjdk.java.net/~gromero/8210320/v2/
I think it's still trivial as before?
Yes.
If so it means I can push it once I receive a second OK from you?
Yes, this is trivial, and AFAIU only one Reviewer is required for trivial issues.
I also think I don't need to push it first to the 'submit' repo since it's a PPC64-only change. Is that correct?
Yes, I don't see the need to test trivial patches like this with submit repo. Thanks, -Aleksey
On 09/04/2018 11:15 AM, Aleksey Shipilev wrote:
On 09/04/2018 04:11 PM, Gustavo Romero wrote:
http://cr.openjdk.java.net/~gromero/8210320/v2/
I think it's still trivial as before?
Yes.
If so it means I can push it once I receive a second OK from you?
Yes, this is trivial, and AFAIU only one Reviewer is required for trivial issues.
Got it. Thanks for confirming it. Either way Martin reviewed it also by now.
I also think I don't need to push it first to the 'submit' repo since it's a PPC64-only change. Is that correct?
Yes, I don't see the need to test trivial patches like this with submit repo.
OK. Thanks! Best regards, Gustavo
Thanks, -Aleksey
Hi Gustavo, it's not a real bug, just a build warning. But it needs to get fixed. Thanks for doing it. Reviewed. Best regards, Martin -----Original Message----- From: hotspot-compiler-dev <hotspot-compiler-dev-bounces@openjdk.java.net> On Behalf Of Gustavo Romero Sent: Dienstag, 4. September 2018 15:42 To: hotspot-compiler-dev@openjdk.java.net Cc: ppc-aix-port-dev@openjdk.java.net Subject: RFR(xs): 8210320: PPC64: Fix uninitialized variable in C1 LIR assembler code Importance: High Hi, May I please request reviews for this tiny change that fixes two uninitialized variables in PPC64 C1 LIR code? Bug : https://bugs.openjdk.java.net/browse/JDK-8210320 Webrev: http://cr.openjdk.java.net/~gromero/8210320/v1/ GCC 4.8 does not complain about these two uninitialized pointers ('data' and 'md') but more recent versions, like 5.4.0 and 7.3.1, complain about it: In file included from /home/gromero/hg/jdk/jdk/src/hotspot/share/c1/c1_Compilation.hpp:29:0, from /home/gromero/hg/jdk/jdk/src/hotspot/share/precompiled/precompiled.hpp:286: /home/gromero/hg/jdk/jdk/src/hotspot/share/ci/ciMethodData.hpp: In member function ‘void LIR_Assembler::emit_typecheck_helper(LIR_OpTypeCheck*, Label*, Label*, Label*)’: /home/gromero/hg/jdk/jdk/src/hotspot/share/ci/ciMethodData.hpp:595:100: warning: ‘data’ may be used uninitialized in this function [-Wmaybe-uninitialized] int byte_offset_of_slot(ciProfileData* data, ByteSize slot_offset_in_data) { return in_bytes(offset_of_slot(data, slot_offset_in_data)); } ^ /home/gromero/hg/jdk/jdk/src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp:2400:18: note: ‘data’ was declared here ciProfileData* data; ^ /home/gromero/hg/jdk/jdk/src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp:2483:78: warning: ‘md’ may be used uninitialized in this function [-Wmaybe-uninitialized] type_profile_helper(mdo, mdo_offset_bias, md, data, recv, Rtmp1, success); ^ Thank you. Best regards, Gustavo
Hi Martin! On 09/04/2018 11:12 AM, Doerr, Martin wrote:
Hi Gustavo,
it's not a real bug, just a build warning. But it needs to get fixed. Thanks for doing it. Reviewed.
Thanks for reviewing it. Yes, I agree. Btw, I tried to precisely determine which change was introduced in gcc 7.3 (for instance) hoping it was only a matter of an additional -Wextra or -Wall in a gcc spec but it turned out that that was not the case apparently... I could not find a reasonable change in gcc flags or source code that might cause such a warnings when gcc 7.3 is used. I've create a "test case" from JVM code for that [1] (which is still 4.4 MiB since I didn't have the change to prune it further). But curious enough although the following simple code really triggers the same warning _both_ on gcc 4.8 and 7.3 when compiled with: $ g++ -Wuninitialized -O3 mu.cpp -c -o mu mu.cpp: void foo(int x) { printf("%d\n", x+1); } int main(int argc, char** argv) { int x; switch (argc) { case 1: x = 1; break; case 2: x = 4; break; case 3: x = 5; } foo(x); } code [1] only triggers the warning in question when gcc 7.3 is used (using the exact same flags): $ g++ -Wuninitialized -O3 ok.cpp -c -o ok.o Passing '-v' to gcc to check the flags from spec didnt show any clue. Toolchain folks also were not able to tell any differences that could account for that behavior on gcc 7.3 without a detailed look... Anyway, it's only a note :) Thanks. Best regards, Gustavo [1] http://cr.openjdk.java.net/~gromero/misc/ok.cpp
Best regards, Martin
-----Original Message----- From: hotspot-compiler-dev <hotspot-compiler-dev-bounces@openjdk.java.net> On Behalf Of Gustavo Romero Sent: Dienstag, 4. September 2018 15:42 To: hotspot-compiler-dev@openjdk.java.net Cc: ppc-aix-port-dev@openjdk.java.net Subject: RFR(xs): 8210320: PPC64: Fix uninitialized variable in C1 LIR assembler code Importance: High
Hi,
May I please request reviews for this tiny change that fixes two uninitialized variables in PPC64 C1 LIR code?
Bug : https://bugs.openjdk.java.net/browse/JDK-8210320 Webrev: http://cr.openjdk.java.net/~gromero/8210320/v1/
GCC 4.8 does not complain about these two uninitialized pointers ('data' and 'md') but more recent versions, like 5.4.0 and 7.3.1, complain about it:
In file included from /home/gromero/hg/jdk/jdk/src/hotspot/share/c1/c1_Compilation.hpp:29:0, from /home/gromero/hg/jdk/jdk/src/hotspot/share/precompiled/precompiled.hpp:286: /home/gromero/hg/jdk/jdk/src/hotspot/share/ci/ciMethodData.hpp: In member function ‘void LIR_Assembler::emit_typecheck_helper(LIR_OpTypeCheck*, Label*, Label*, Label*)’: /home/gromero/hg/jdk/jdk/src/hotspot/share/ci/ciMethodData.hpp:595:100: warning: ‘data’ may be used uninitialized in this function [-Wmaybe-uninitialized] int byte_offset_of_slot(ciProfileData* data, ByteSize slot_offset_in_data) { return in_bytes(offset_of_slot(data, slot_offset_in_data)); } ^ /home/gromero/hg/jdk/jdk/src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp:2400:18: note: ‘data’ was declared here ciProfileData* data; ^ /home/gromero/hg/jdk/jdk/src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp:2483:78: warning: ‘md’ may be used uninitialized in this function [-Wmaybe-uninitialized] type_profile_helper(mdo, mdo_offset_bias, md, data, recv, Rtmp1, success); ^
Thank you.
Best regards, Gustavo
participants (4)
-
Aleksey Shipilev
-
Baesken, Matthias
-
Doerr, Martin
-
Gustavo Romero