Recent

Author Topic: Is there a way to detect "Trash local variables" at compile time?  (Read 13850 times)

Thaddy

  • Hero Member
  • *****
  • Posts: 14197
  • Probably until I exterminate Putin.
Re: Is there a way to detect "Trash local variables" at compile time?
« Reply #30 on: July 12, 2018, 06:29:31 pm »
Do you think this code is a problematic code?
Code: Pascal  [Select][+][-]
  1. procedure Test(const R: TR; out O: TR);
  2. begin
  3.   O := R;
  4. end;
In fact, the procedure can be quite correctly handle the situation when the const and out parameter is the same (the same memory area), but to report this to the options -gt fails.
No. It is not correct. The compiler should make a copy in this case. (or warn).
<sigh, sorry> This is about deep copy and shallow copy after all. That's why I hate C++ and still teach it.
« Last Edit: July 12, 2018, 06:32:23 pm by Thaddy »
Specialize a type, not a var.

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 9791
  • Debugger - SynEdit - and more
    • wiki
Re: Is there a way to detect "Trash local variables" at compile time?
« Reply #31 on: July 12, 2018, 07:19:16 pm »
As I understand now there is no such way, and if someone gets an obvious error using my code, then I will explain to him that so use the procedure is not very good, because it is not reflected in the language and documentation as a bad side effect.
My tests shows he has a rather liberate opinion of truth... He did not test. I did.
You misquoted. I did not say, what you attributed to me. I myself quoted it (in order to reply to it), and it was marked as such.

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 9791
  • Debugger - SynEdit - and more
    • wiki
Re: Is there a way to detect "Trash local variables" at compile time?
« Reply #32 on: July 12, 2018, 07:19:21 pm »
Code: Pascal  [Select][+][-]
  1. procedure Test(const R: TR; out O: TR);
  2. begin
  3.   O := R;
  4. end;
In fact, the procedure can be quite correctly handle the situation when the const and out parameter is the same (the same memory area), but to report this to the options -gt fails.
In current fpc, or in all future?

But start with todays fpc. If "TR" include ref counted types (or ever will in future):
- const will not increase the ref count
- out may clear refcounted data before it passes it in (it may not always do, not sure, but afaik it can)

So if  R.SomeAnsiString has a refcount of 1, then const will leave it at 1, but out will reduce it. The string will be released before being passed in, and that is for both parameters.

The const param (if passed by value), may still have the pointer to where the string was. So this could even crash. Not necessarily in Test, but at some time later.



440bx

  • Hero Member
  • *****
  • Posts: 3944
Re: Is there a way to detect "Trash local variables" at compile time?
« Reply #33 on: July 12, 2018, 08:24:48 pm »
And (according to the doc) you promised to the compiler, that you would write the code in such way that this would not happen (neither directly nor by side effect).

Martin,

What you point out in that statement is absolutely correct BUT, it's not only the user that has to respect that promise, the compiler is the first one that is expected to honor that promise.  The -gt switch causes the compiler to emit code that overwrites the const parameter, thereby causing all the problems.

-gt causes the compiler to break the promise it expects the user to uphold.   What that switch is doing, admittedly with good intentions, is clearly incorrect.


(FPC v3.0.4 and Lazarus 1.8.2) or (FPC v3.2.2 and Lazarus v3.2) on Windows 7 SP1 64bit.

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 9791
  • Debugger - SynEdit - and more
    • wiki
Re: Is there a way to detect "Trash local variables" at compile time?
« Reply #34 on: July 12, 2018, 08:44:13 pm »
And (according to the doc) you promised to the compiler, that you would write the code in such way that this would not happen (neither directly nor by side effect).

Martin,

What you point out in that statement is absolutely correct BUT, it's not only the user that has to respect that promise, the compiler is the first one that is expected to honor that promise.  The -gt switch causes the compiler to emit code that overwrites the const parameter, thereby causing all the problems.

-gt causes the compiler to break the promise it expects the user to uphold.   What that switch is doing, admittedly with good intentions, is clearly incorrect.

Well, that only comes into play, if the the "out param" is otherwise never written too.

So yes, if I declare an out param, to which I never write, then it is this switch that brakes my promise -> alternative reading: I broke the promise by using the switch.

But if I declare an out param, I am likely to write to it, am I not? (Yeah, other cases can be constructed, like check that the const param is different before writing the out param / But even that breaks, see my prev post).
If indeed I am writing to the out param, the all -gt does is, it pulls the broken promise to the top of the procedure, so it is easier to spot.

-------------
But yes it brings up the OP request.

There is $R- to disable range checking (because indeed there is valid code, that gives false positives on range checking).

Now if valid code can be found that relies on -gt off, then a flag for this would be needed.
If I am not mistaken, we did not yet have such valid code in this thread. (But I did not double check)
valid means: not relying on the implementation detail of the current compiler, but on any code that any future fpc may generate within the bounds of documentation. (and where the fpc team does not declare that the doc itself is buggy)

440bx

  • Hero Member
  • *****
  • Posts: 3944
Re: Is there a way to detect "Trash local variables" at compile time?
« Reply #35 on: July 12, 2018, 09:36:07 pm »
Well, that only comes into play, if the the "out param" is otherwise never written too.

No, because the code that implements the -gt switch will have corrupted the parameters before the user/programmer gets a chance to do it.

So yes, if I declare an out param, to which I never write, then it is this switch that brakes my promise -> alternative reading: I broke the promise by using the switch.
No, the switch breaks the promise whenever the function/procedure is called with both arguments referring to the same variable.  In that case, which is the case Serge has demonstrated, that switch breaks the promise before the user has a chance to. 

What I'm going to say next is admittedly arguable but, it can be argued that Serge didn't break the promise at all since he is simply assigning the variable to itself.   He doesn't change the value, the value remains constant.  That said, I will be the first to point out and admit that just writing to what is supposed to be a constant is questionable but, that's what -gt does and it does it without respecting the value of what is supposed to be constant.

But if I declare an out param, I am likely to write to it, am I not? (Yeah, other cases can be constructed, like check that the const param is different before writing the out param / But even that breaks, see my prev post).
If indeed I am writing to the out param, the all -gt does is, it pulls the broken promise to the top of the procedure, so it is easier to spot.
No because the programmer isn't creating a problem by writing the same value onto itself (the promise is kept that way).  -gt on the other hand, tramples on the constant by overwriting it with an arbitrary value of its choice.


This is a typical pointer aliasing problem and -gt a priory assumed that there was no aliasing.  What it does _depends_ on the parameters not being aliased, whereas what Serge is doing does not.  He is writing to a const (which is admittedly questionable) but, unlike the -gt switch he preserves the value which meets the essence of the promise made to the compiler.

A compiler should not assume that a pointer and a variable or two pointers are not aliases of each other unless it can deterministically determine they are not.    Serge's procedure call is perfectly valid, the assumption made by the -gt switch isn't, thus leading to an incorrect result.

That behavior of the -gt switch is, and should be considered a bug.  It unwarrantedly assumed that the parameters are not aliases of each other thus causing a problem.

(FPC v3.0.4 and Lazarus 1.8.2) or (FPC v3.2.2 and Lazarus v3.2) on Windows 7 SP1 64bit.

ASerge

  • Hero Member
  • *****
  • Posts: 2222
Re: Is there a way to detect "Trash local variables" at compile time?
« Reply #36 on: July 12, 2018, 09:57:48 pm »
If "TR" include ref counted types (or ever will in future):
- const will not increase the ref count
- out may clear refcounted data before it passes it in (it may not always do, not sure, but afaik it can)
1. If (as Spartans  :))
2. With counted types option -gt don't fill out parameters.
Code: Pascal  [Select][+][-]
  1. {$MODE OBJFPC}
  2. {$LONGSTRINGS ON}
  3. {$APPTYPE CONSOLE}
  4.  
  5. type
  6.   TR = record
  7.     X: Integer;
  8.     S: string;
  9.   end;
  10.  
  11. procedure Test(const Rin: TR; out Rout: TR);
  12. begin
  13.   Writeln(StringRefCount(Rin.S));
  14.   Rout := Rin;
  15.   Writeln(StringRefCount(Rin.S));
  16. end;
  17.  
  18. var
  19.   R: TR;
  20. begin
  21.   R.S := 'Some string';
  22.   R.X := 1;
  23.   Test(R, R);
  24.   Writeln(R.S, ' ', R.X);
  25.   Readln;
  26. end.
R.X will be output as 1 (of course, S is empty as you mentioned above). But, if we comment out the field S, the garbage will be printed again.
I think:
1. Using const and out parameters - it's normal. In this case, if the types are the same, the developer should take into account that they can pass the same value (pointer).
2. Option -gt poorly documented. I.e. it is not clear what and in what cases it does.
3. If it makes the worked code non-functional, then there must be a possibility of disabling it locally or at least defining it to issue a warning.
4. Perhaps the compiler should issue a warning if the same variable is used more than once when calling procedure, provided that one of the parameters is out. And make a temporary copy of the variable to eliminate the side effect.

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 9791
  • Debugger - SynEdit - and more
    • wiki
Re: Is there a way to detect "Trash local variables" at compile time?
« Reply #37 on: July 12, 2018, 10:02:33 pm »
Well, that only comes into play, if the the "out param" is otherwise never written too.
No, because the code that implements the -gt switch will have corrupted the parameters before the user/programmer gets a chance to do it.
That is the entire intent of -gt.
If you have code, that may crash or misbehave (sometimes, or even rarely), -gt attempts to increase the change that this code will crash or misbehave.
The exact line is not guaranteed, but that is similar (at least in some cases) when it is a local var that was trashed.
Though I grant you, wish a trashed local var, the error usually happens either at or (far) after the access to that variable. And here it happen earlier. But still the goal is archived, you get an indication that something is wrong with your code.

Quote
So yes, if I declare an out param, to which I never write, then it is this switch that brakes my promise -> alternative reading: I broke the promise by using the switch.
No, the switch breaks the promise whenever the function/procedure is called with both arguments referring to the same variable.  In that case, which is the case Serge has demonstrated, that switch breaks the promise before the user has a chance to. 
Again, so what? Goal archived, the user knows there is an error. Better than shipping a broken app, and learn from your customer that it is broken.

[/quote]
What I'm going to say next is admittedly arguable but, it can be argued that Serge didn't break the promise at all since he is simply assigning the variable to itself.   He doesn't change the value, the value remains constant.  That said, I will be the first to point out and admit that just writing to what is supposed to be a constant is questionable but, that's what -gt does and it does it without respecting the value of what is supposed to be constant.
[/quote]
Ok, that is somewhat valid. (Except for refcounted types, because the assignment may change ref counts)

I am somehow not sure how far that will carry as an argument to get a {$GT-} directive (which is what would be needed in that case)


Quote
But if I declare an out param, I am likely to write to it, am I not? (Yeah, other cases can be constructed, like check that the const param is different before writing the out param / But even that breaks, see my prev post).
If indeed I am writing to the out param, the all -gt does is, it pulls the broken promise to the top of the procedure, so it is easier to spot.
No because the programmer isn't creating a problem by writing the same value onto itself (the promise is kept that way).  -gt on the other hand, tramples on the constant by overwriting it with an arbitrary value of its choice.
<repeated>

Quote
This is a typical pointer aliasing problem and -gt a priory assumed that there was no aliasing.  What it does _depends_ on the parameters not being aliased, whereas what Serge is doing does not.  He is writing to a const (which is admittedly questionable) but, unlike the -gt switch he preserves the value which meets the essence of the promise made to the compiler.
Again not for refcounted types.

But yes the doc currently states "will not be changed". So maybe that should read " will not be written to"
E.g if I have "const A=5;" I can not later in code do "A:=5" even though it would not change the value of the code.

Anyway I can only guess here....
I remember the "const param" docs had to be updated before, because they where not accurate enough. But maybe this time they are... I don't know that.


Quote
A compiler should not assume that a pointer and a variable or two pointers are not aliases of each other unless it can deterministically determine they are not.    Serge's procedure call is perfectly valid, the assumption made by the -gt switch isn't, thus leading to an incorrect result.

That behavior of the -gt switch is, and should be considered a bug.  It unwarrantedly assumed that the parameters are not aliases of each other thus causing a problem.
Well, 50% disagree.
That is, it is a bug, but it might be a bug in the doc. It may be that this behaviour is missing from the doc. But I did not design it, so I can not say what the intend is.

And again, if passing in a ref counted value in this Test function, then it may be that the problem happens even without -gt. Again I dont know what the indent here is.
But previous discussions on the "const param" left me with the impression, that it is meant to leave all burden to the user. (simple stating this, not judging it)

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 9791
  • Debugger - SynEdit - and more
    • wiki
Re: Is there a way to detect "Trash local variables" at compile time?
« Reply #38 on: July 12, 2018, 10:38:25 pm »
Interesting the string is empty with a constant as string. constant strings have a special refcount value (afaik). That is why I usually use "copy('foo-',1,3);"

R.X will be output as 1 (of course, S is empty as you mentioned above). But, if we comment out the field S, the garbage will be printed again.
I think:
1. Using const and out parameters - it's normal. In this case, if the types are the same, the developer should take into account that they can pass the same value (pointer).
2. Option -gt poorly documented. I.e. it is not clear what and in what cases it does.
3. If it makes the worked code non-functional, then there must be a possibility of disabling it locally or at least defining it to issue a warning.
4. Perhaps the compiler should issue a warning if the same variable is used more than once when calling procedure, provided that one of the parameters is out. And make a temporary copy of the variable to eliminate the side effect.

1) But with ref counts, anything you do in the called function is too late.  See more below
2) yes
3) See issue about "const param" above. If the doc stays, then probably yes: working code. Otherwise working by accident, but not documentation, which breaks by lots of things.
4) +1


Code: Pascal  [Select][+][-]
  1. {$MODE OBJFPC}
  2. {$LONGSTRINGS ON}
  3. {$APPTYPE CONSOLE}
  4.  
  5. type
  6.   TR = record
  7.     X: Integer;
  8.     S: string;
  9.   end;
  10.  
  11. procedure Test(constref Rin: TR; out Rout: TR);
  12. begin
  13.   Writeln(StringRefCount(Rin.S),ptrint(pointer(rin.s)));
  14. end;
  15.  
  16. var
  17.   R: TR;
  18. begin
  19.   R.S := 'abcdef';
  20.   R.S[1] := 'a';  // ref count to 1
  21.   R.X := 1;
  22.  
  23.   Writeln(StringRefCount(R.S));
  24.   Test(R, R);
  25.   Writeln(StringRefCount(R.S));  // 0 // even the callers value was damaged
  26.   Readln;
  27. end.

But it gets better (tested with 32 bit)
Code: Pascal  [Select][+][-]
  1. {$MODE OBJFPC}
  2. {$LONGSTRINGS ON}
  3. {$APPTYPE CONSOLE}
  4.  
  5. function Test(out Rout: string; const Rin: string): string;
  6. begin
  7.   Writeln(StringRefCount(Rin), ', ',ptrint(pointer(rin)));
  8.   Result := rin;
  9. end;
  10.  
  11. var
  12.   R, x: string;
  13. begin
  14.   R := 'abcdef';
  15.   R[1] := 'a';
  16.  
  17.   Writeln(StringRefCount(R));
  18.   x:=Test(R, R);
  19.   Writeln(StringRefCount(R));
  20.   Writeln(StringRefCount(x), ', ',ptrint(pointer(x)));
  21.   Readln;
  22. end.
Rin points to memory that has been freed. Accessing it could have all sorts of bad effects. E.g

Same for x => crash on app exit.
If you where to do x[1]:= 'k'; then that  may also crash....

(I had the whole const param with ref counts before. So I know quite well how to get it to go wrong....)

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 9791
  • Debugger - SynEdit - and more
    • wiki
Re: Is there a way to detect "Trash local variables" at compile time?
« Reply #39 on: July 12, 2018, 10:42:29 pm »
But there may be a good argument for being able to disable -gt locally.

Not tested, but
Code: Pascal  [Select][+][-]
  1. function Test(out Rout: TC; var Rin: TC);

would not violate any "const" promise. But if it suffers the same -gt, then -gt certainly went wrong. (IMHO)

BeniBela

  • Hero Member
  • *****
  • Posts: 905
    • homepage
Re: Is there a way to detect "Trash local variables" at compile time?
« Reply #40 on: July 13, 2018, 02:00:53 am »
Maybe there generally is room to lobby for a note/hint from the compiler for any case where the same variable is passed twice or more by reference to the same procedure.

Definitely

But it should be a warning. Or even an error


This is an awful trap.


Rule of thumb would be to never pass the same variable twice to a function. Not as result either. Even if you check that it is ok, an update could later change a copy parameter to a const parameter

PascalDragon

  • Hero Member
  • *****
  • Posts: 5446
  • Compiler Developer
Re: Is there a way to detect "Trash local variables" at compile time?
« Reply #41 on: July 16, 2018, 10:56:02 pm »
Do you think this code is a problematic code?
Code: Pascal  [Select][+][-]
  1. procedure Test(const R: TR; out O: TR);
  2. begin
  3.   O := R;
  4. end;
In fact, the procedure can be quite correctly handle the situation when the const and out parameter is the same (the same memory area), but to report this to the options -gt fails.
No. It is not correct. The compiler should make a copy in this case. (or warn).
<sigh, sorry> This is about deep copy and shallow copy after all. That's why I hate C++ and still teach it.
The function is correct. It's the caller that must pay attention to what it is passing to Test.
As stated in the documentation a const-parameter means a contract, namely that the value passed in isn't changed. Now a out-parameter (or even a var-parameter) is essentially always written to. If one now passes the same variable to both parameters the contract is broken. Simple as that. It would be the same if Test manipulated a global variable that is also passed in as const-parameter.
And the compiler should not need to do a copy in this case, because an idea behind the const is that the compiler can optimize away such copies!

 

TinyPortal © 2005-2018