| Message ID | 20240502185924.2060196-4-vineetg@rivosinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Miscll fixlets | expand |
On 5/2/24 12:59 PM, Vineet Gupta wrote: > This is no logic change (but technically still a functional change). > > Ran into this when stepping thru combine code. > @newpat has some random garbage for a bit until it is actually set. > With the fix it remains 0 until actually set. > > gcc/ChangeLog: > * combine.cc (try_combine): Initialize newpat. Isn't the same true even after this change if you turn on the optimizer? And isn't the same true for many other objects that are initialized lazily? Ultimately I'll defer to Segher on this. jeff
On 5/2/24 13:38, Jeff Law wrote: > > On 5/2/24 12:59 PM, Vineet Gupta wrote: >> This is no logic change (but technically still a functional change). >> >> Ran into this when stepping thru combine code. >> @newpat has some random garbage for a bit until it is actually set. >> With the fix it remains 0 until actually set. >> >> gcc/ChangeLog: >> * combine.cc (try_combine): Initialize newpat. > Isn't the same true even after this change if you turn on the optimizer? You are right. And the reason I was seeing a difference at all is all my local toolchain builds are -g3 -O0 :-) > And isn't the same true for many other objects that are initialized > lazily? > > Ultimately I'll defer to Segher on this. Of course. Thx for taking a look. -Vineet
On Thu, May 02, 2024 at 11:59:24AM -0700, Vineet Gupta wrote: > This is no logic change (but technically still a functional change). Where are 1/3 and 2/3? Or are those unrelated? Please don't make series like that. > Ran into this when stepping thru combine code. > @newpat has some random garbage for a bit until it is actually set. > With the fix it remains 0 until actually set. The same is true for all uninitialised variables. Setting everything to zero explicitly is a) quite a bit slower, and b) just as wrong! For example, here, newpat should never be zero. Never. It does not make any sense. Is there any place where newpat is used uninitialised? Segher
On Thu, May 02, 2024 at 02:38:12PM -0600, Jeff Law wrote: > > > On 5/2/24 12:59 PM, Vineet Gupta wrote: > >This is no logic change (but technically still a functional change). > > > >Ran into this when stepping thru combine code. > >@newpat has some random garbage for a bit until it is actually set. > >With the fix it remains 0 until actually set. > > > >gcc/ChangeLog: > > * combine.cc (try_combine): Initialize newpat. > Isn't the same true even after this change if you turn on the optimizer? > And isn't the same true for many other objects that are initialized > lazily? For all, even. Without this change the compiler can (in theory, anyway) diagnose the uninitialised use. After, there *is* no uninitialised use anymore. Please don't do this. It is not an improvement, it is several steps back, to satisfy a misguides sense of "security". Segher
On 5/3/24 01:26, Segher Boessenkool wrote: > On Thu, May 02, 2024 at 11:59:24AM -0700, Vineet Gupta wrote: >> This is no logic change (but technically still a functional change). > Where are 1/3 and 2/3? Or are those unrelated? Yes they were unrelated (minor doc fixes) hence didn't want to spam you, but you are right just because they were piled up in my spring cleaning branch doesn't mean I send them out that way. > Please don't make series like that. Noted for future. > >> Ran into this when stepping thru combine code. >> @newpat has some random garbage for a bit until it is actually set. >> With the fix it remains 0 until actually set. > The same is true for all uninitialised variables. Setting everything > to zero explicitly is a) quite a bit slower, and b) just as wrong! > For example, here, newpat should never be zero. Never. It does not > make any sense. > > Is there any place where newpat is used uninitialised? As I mentioned this patch was the outcome of my "debugging" experience and agree I should not conflate between what the compiler does and what is needed to debug the compiler itself. I've dropped this one. Thx, -Vineet
diff --git a/gcc/combine.cc b/gcc/combine.cc index 92b8d98e6c15..0b5fe00c8c5b 100644 --- a/gcc/combine.cc +++ b/gcc/combine.cc @@ -2522,7 +2522,7 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0, bool *new_direct_jump_p, rtx_insn *last_combined_insn) { /* New patterns for I3 and I2, respectively. */ - rtx newpat, newi2pat = 0; + rtx newpat = 0, newi2pat = 0; rtvec newpat_vec_with_clobbers = 0; bool substed_i2 = false, substed_i1 = false, substed_i0 = false; /* Indicates need to preserve SET in I0, I1 or I2 in I3 if it is not
This is no logic change (but technically still a functional change). Ran into this when stepping thru combine code. @newpat has some random garbage for a bit until it is actually set. With the fix it remains 0 until actually set. gcc/ChangeLog: * combine.cc (try_combine): Initialize newpat. CC: Segher Boessenkool <segher@kernel.crashing.org> Signed-off-by: Vineet Gupta <vineetg@rivosinc.com> --- gcc/combine.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)