Is the below a BUG (I assume it is) Shall I put it in mantis?

Below is
- a small function
- an extract of what fpc generates with -la
- from the -la output, the debug info

This was generated without optimization

the statement for "i:=1" ends after " movl $1,%eax ". it ends before the value is written to memory (the "movl %eax,-12(%ebp)" is part of the next command)

This means the debugger can not show the correct value.

See also http://forum.lazarus.freepascal.org/index.php/topic,22598.0.html

procedure TForm1.Button1Click(Sender: TObject);
var
   i: Integer;
begin
   i := 1;
   // After this line Delphi Evaluate/Modify shows '1', Lazarus still a
random (uninitialized?) integer
   i := i + 1;
   // Ater this line Delphi & Lazarus both show '2'
   ShowMessageFmt('%d',[i]);
end;

-------------------------------------------
.Ll3:
# [36] i := 1;
movl $1,%eax
.Ll4:
# [38] i := i + 1;
movl %eax,-12(%ebp)
addl $1,%eax
jno .Lj11
call FPC_OVERFLOW
.Lj11:
movl %eax,-12(%ebp)
.Ll5:

---------------------------------------
# file_names
.ascii "unit1.pas\000"
.uleb128 0
.uleb128 0
.uleb128 0
.byte 0
.Lehdebug_line0:
# === header end ===
# function: UNIT1$_$TFORM1_$__$$_BUTTON1CLICK$TOBJECT # [35:1]
.byte 0
.uleb128 5
.byte 2
.long .Ll1
.byte 5
.uleb128 1
.byte 46
# [35:1]
.byte 2
.uleb128 .Ll2-.Ll1
.byte 1
# [36:3]
.byte 2
.uleb128 .Ll3-.Ll2
.byte 5
.uleb128 3
.byte 13
# [38:10]
.byte 2
.uleb128 .Ll4-.Ll3
.byte 5
.uleb128 10
.byte 14
# [40:26]
.byte 2
.uleb128 .Ll5-.Ll4
.byte 5
.uleb128 26
.byte 14

If you disable optimisations it will work as you expect. Even -O1 can slightly mess up the debug information in some cases.

Jonas

Sorry my fault,

I had thought that I disabled all optimisation.
But I must have had some config in the depth of my IDE.

Using a clean IDE conf, and yes it works with optimization disabled.

It is still interesting though.

Why is the
movl %eax,-12(%ebp)
generated at the start of i:=i+1

Why does a statement save an outdated value to memory?

It does not read it back, it continues with what is already on memory, and the at the end of the statement (this time in the statement) the new value is writtes to memory.

Meaning "i := i+1" writes something to memory, that will never be used,
and be replaced within the same statement.

I agree that -O1 is not guaranteed save (and I had tried to test it, but got caught by an IDE that outsmarted me...)

So it is ok for O1 to drop the
movl %eax,-12(%ebp)
from the "i:=1"
I accept that.

But then for an optimization, to add a useless asm to the next command?

Because the load back from memory that came right after it got removed by a peephole optimisation. Peephole optimisations are by definition local and have no clue about what happens next, and hence the store remains.

Jonas

Ok, i understand, but that would mean hat the save originally was
intended for the "i := 1" statement.

So somewhere in that process, the debug info, and the statements get out of sync.

I only wonder, if despite the fact that O1 does not guarantee, if in this case it would be worth to consider it a bug (and intend to fix it)?

Because a lot of users rely on O1 being good for the debugger, and not everyone might spot such a value as wrong by the debugger. So people may spent a long time to figure out why there code does something, that it actually does not do.

The transformation that gets applied is

movl const1,(mem1)
movl (mem1),reg1
to:
movl const1,reg1
movl reg1,(mem1) }

The second instruction originally belonged to the second statement and hence has its line number info.

I only wonder, if despite the fact that O1 does not guarantee, if in this case it would be worth to consider it a bug (and intend to fix it)?

If someone wants to change it, they're free to do so. My days of working on the peephole optimisers are over.

Jonas

Well I gave it a try, and now would appreciate feedback. (patch at end of message).

It basically just copies the fileinfo record to the 2nd tai statement. (must be the complete info (or maybe at least line and filename), for include files). In theory that could lead to the statement in the next line generating no code (if other optimizations take away whatever else it did. But that would be no damage as far as I can see. (not sure if it can even happen in real live)

Appears to work with stabs and dwarf. Also works if the next statement is inlined (independent of the inlined proc being from the same or a diff unit)

procedure Foo(var x:Integer); inline;
   begin  x := x + 1;  end;
function Bar(var x:Integer): integer; inline;
   begin  result := x + 1;  end;
   i := 1;
   i := i + 1;
   i := 1;
   i := bar(i);
   i := 1;
   foo(i);

If the statements are on the same line, the go together anyway (also with inlined)

   i := 1;   i := i + 1;

And also ok, if 2nd line is in include file
   i := 1;
   {$i u1.inc}

Currently the ait_label with labeltype = alt_dbgline for debugline-info, are generated after the optimization. Can that be relied on? If that changes the fix would be completely different (would need to move the generated label).
Also the debug-info generating code only looks of line info of ait_instruction, so any skipped TAI entries (comments, ...) do not need an update.

There may be other similar optimizations, but one step at a time.

Not sure how (or where) to write a test. Are there examples that show, how to test that debug info and generated assembler are correctly fitting together?
(Existing tests are still running, but since this is for feedback, the results can be submitted later)

Index: compiler/i386/popt386.pas
=================================================================== --- compiler/i386/popt386.pas (revision 26518)
+++ compiler/i386/popt386.pas (working copy)
@@ -1401,6 +1401,8 @@
taicpu(hp1).loadReg(0,taicpu(hp1).oper[1]^.reg);
taicpu(hp1).loadRef(1,taicpu(p).oper[1]^.ref^);
taicpu(p).loadReg(1,taicpu(hp1).oper[0]^.reg);
+ // Storing the value to memory should be part of the first instruction
+ taicpu(hp1).fileinfo := taicpu(p).fileinfo;

                                  end
                          end;
                        if GetNextInstruction(p, hp1) and

Martin wrote

(Existing tests are still running, but since this is for feedback, the results can be submitted later)

UNMODIFIED TRUNK rev 26506:

utils/digest output/i386-win32/log
Total = 6874 (24:6850)
Total number of compilations = 4177 (7:4170)
Successfully compiled = 3146
Successfully failed = 1024
Compilation failures = 4
Compilation that did not fail while they should = 3 Total number of runs = 2697 (17:2680)
Successful runs = 2680
Failed runs = 17
Number units compiled = 132
Number program that should not be run = 314
Number of skipped tests = 240
Number of skipped graph tests = 10
Number of skipped interactive tests = 30
Number of skipped known bug tests = 6
Number of skipped tests for other versions = 4
Number of skipped tests for other cpus = 53
Number of skipped tests for other targets = 137

TRUNK with peephole change rev 26518 (12 revisions later) utils/digest output/i386-win32/log
Total = 6874 (19:6855)
Total number of compilations = 4177 (7:4170)
Successfully compiled = 3146
Successfully failed = 1024
Compilation failures = 4
Compilation that did not fail while they should = 3 Total number of runs = 2697 (12:2685)
Successful runs = 2685
Failed runs = 12
Number units compiled = 132
Number program that should not be run = 314
Number of skipped tests = 240
Number of skipped graph tests = 10
Number of skipped interactive tests = 30
Number of skipped known bug tests = 6
Number of skipped tests for other versions = 4
Number of skipped tests for other cpus = 53
Number of skipped tests for other targets = 137

Since I have failing tests either way, it is hard to say anything about the change.

Hi Martin,

if you want to compare to test runs,
you should save at least three files at the end of each run: output/$fpctarget/faillist
output/$fpctarget/log
and
output/$fpctarget/longlog

The best is to first compare the two log files with diff utility.
Inside longlog, you should be able to find more information about the differences.

In the hope this can help you,

Pierre Muller

Thanks.

Eventually I will need to run them from the same checkout. I run the same revision, but from 2 checkouts (for convenience), but I guess some option to make must have differed.

My modifications fail 5 tests less. They are peephole opt, they should not fix anything, and I do doubt that they did.

Starting one by one.

This is the simplest of the changes. In addition to the patch in this message, what is needed to get it committed?

Well I gave it a try, and now would appreciate feedback. (patch at end of message).

It basically just copies the fileinfo record to the 2nd tai statement. (must be the complete info (or maybe at least line and filename), for include files). In theory that could lead to the statement in the next line generating no code (if other optimizations take away whatever else it did. But that would be no damage as far as I can see. (not sure if it can even happen in real live)

Appears to work with stabs and dwarf. Also works if the next statement is inlined (independent of the inlined proc being from the same or a diff unit)

procedure Foo(var x:Integer); inline;
   begin  x := x + 1;  end;
function Bar(var x:Integer): integer; inline;
   begin  result := x + 1;  end;
   i := 1;
   i := i + 1;
   i := 1;
   i := bar(i);
   i := 1;
   foo(i);

If the statements are on the same line, the go together anyway (also with inlined)

   i := 1;   i := i + 1;

And also ok, if 2nd line is in include file
   i := 1;
   {$i u1.inc}

Currently the ait_label with labeltype = alt_dbgline for debugline-info, are generated after the optimization. Can that be relied on? If that changes the fix would be completely different (would need to move the generated label).
Also the debug-info generating code only looks of line info of ait_instruction, so any skipped TAI entries (comments, ...) do not need an update.

There may be other similar optimizations, but one step at a time.

Not sure how (or where) to write a test. Are there examples that show, how to test that debug info and generated assembler are correctly fitting together?
(Existing tests are still running, but since this is for feedback, the results can be submitted later)

Index: compiler/i386/popt386.pas
=================================================================== --- compiler/i386/popt386.pas (revision 26518)
+++ compiler/i386/popt386.pas (working copy)
@@ -1401,6 +1401,8 @@
taicpu(hp1).loadReg(0,taicpu(hp1).oper[1]^.reg);
taicpu(hp1).loadRef(1,taicpu(p).oper[1]^.ref^);
taicpu(p).loadReg(1,taicpu(hp1).oper[0]^.reg);
+ // Storing the value to memory should be part of the first instruction
+ taicpu(hp1).fileinfo := taicpu(p).fileinfo;

                                  end
                          end;
                        if GetNextInstruction(p, hp1) and