http://bugs.freepascal.org/view.php?id=19605
It may be easier to discuss here.

* First it needs to be decided, if such a check in itself (ignoring any obstacles in it's implementation) has a chance to be such a check is not wanted (even if implementation were trivial), then well that's it.

* If it could be accepted as a feature (request), then I understand it may still be open for very long, as there is no guarantee, that anyone will do it.

* Implementation

On top of what I already wrote on mantis. I believe my initial idea can be further simplified.

Given:  procedure Foo(const s1, s2: string);

1) On entering of Foo:
- Foo needs a numeric temp variable for each passed in ref-counted const param
- The current ref count of each such param is stored in that list of temp vars

       for each sx in [s1, s2] do "tmp_rc[sx] := refcnt(sx)"

- The ref-count of each param is increased
There is no need to check if s1=s2; if so then the ref-count of both of them will have increased by 2

       for each sx in [s1, s2] do "refcnt(sx) := refcnt(sx) + 1"

- The temp vars are updated, to become the difference between the original, and the new recount

       for each sx in [s1, s2] do "tmp_rc[sx] := refcnt(sx) - tmp_rc[sx]"

2) On exit, all the tep ref counts are decreased

       for each sx in [s1, s2] do "refcnt(sx) := refcnt(sx) - 1"

No ref count is allowed to become less than 1.
If any refcount goes less than 1, than this is an error.

3)
On any statement inside Foo, that adds a ref count to any Sx ("other := s1"), the refcount of this sx has to be checked before the statement if refcnt(sx) <= tmp_rc[sx] then error

       other := sx

This way, if at any time, all non-temporary ref counts where gone, they can not be re-added ever again

It still requires, that, if any such param is passed to another procedure, that will accept it as const param, then all temp ref-counts must be removed.
- That is only, if the called procedure, is compiled with the flag, and adds it-s own temp refcnts
- If the called procedure is not compiled with the flag, then they must be kept.
Otherwise check "3" will not hold in a nested call.

In order do to such tmp-ref-cnt removal for selected params only, one would have to check for equality of multiply passes const param again. (Undesirable)

It might be easier, if a procedure
   procedure Foo(const s1, s2: string);
when compiled with the check-flag, wll have some hidden extra param
procedure Foo(const s1, s2: string; tmp_ref_cnt_s1, tmp_ref_cnt_s2:
Integer);

They contain the count of already added tmp ref-cnts ("tmp_rc[sx] "); or zero. This can then be use to check for the correct value, in point "3".

Personally I don't see the need for such a check because I consider it as a seldom corner case but I can understand that people might be interested in it so if somebody implements it properly, I'am pretty sure it can be included.

That's what I fear :) If already the first draft looks so complicated, reality is typically much worse.

What about records, arrays etc. containing ref. counted types?

Florian Klämpfl wrote

Personally I don't see the need for such a check because I consider it as a seldom corner case but I can understand that people might be interested in it so if somebody implements it properly, I'am pretty sure it can be included.

It may bne rare, sure. But it is also one that is very hard to find. Because code like this may run without crash, for a long time, and by the time one notices it, it is hard to track. And *if* const param are used, it is easy to sneak in, even if the author is available of it (which I believe very very few people are)

Very true :(

This case is shlighly different.

You do not have an additional pointer to the actual char data (which is missing the ref count)

You have a pointer to the record (or whatever), and in there you have the original pointer to the char data. So the non increase of the ref count is correct, and needs no checks. (same as if the record was a var param)
True the original string (if global) may become nil or change to another mem location => but then so does the value in the "const record"

The check is about the ref-counting going wrong.
I already established that content change catching is a side effect, caused by copy on write (dynamic arrays would be able to do ref counts check, but would not notice content changes.

Fetching content changes, would require to either copy or checksum *every* const param => that is a different beast (one that I did not propose...)

Martin wrote

It may bne rare, sure. But it is also one that is very hard to find. Because code like this may run without crash, for a long time, and by the time one notices it, it is hard to track. And *if* const param are used, it is easy to sneak in, even if the author is available of it (which I believe very very few people are)

FWIW: Valgrind should also be able to identify such errors (of course, it requires that you are on a platform that's supported by Valgrind)

Jonas

I suppose Florian already knows that, he just meant it's basically the same programming/coding error. And also guess he meant "where do we stop"? ;-)
Maybe just adding the option to disable the const optimization and letting the programmer figure out whether it fixes or not a bug is a good enough addition?

Best regards,
Flávio

In our previous episode, Jonas Maebe said:

> It may bne rare, sure. But it is also one that is very hard to find. Because code like this may run without crash, for a long time, and by the time one notices it, it is hard to track. And *if* const param are used, it is easy to sneak in, even if the author is available of it (which I believe very very few people are) FWIW: Valgrind should also be able to identify such errors (of course, it requires that you are on a platform that's supported by Valgrind)

And the problem with the profiler is often that their slowness (both startup and during production run) prohibits testing at the client's setup. Moreover their operation requires skill. I've such problems both with AQTime and even with remote debugging Delphi apps.

While a binary with certain checks compiled in can be run by the client, though of course it must be possible to create some kind of log in such case.

May I propose a more generic solution:
- with some switch (-gc?) a call to checkpointer is inkoved at the end of a procedure for const ref. counted types. Together with -gh this catches the common case:

{$h+}
var
  s : string;
procedure proc;
  begin
    s:='1234';
  end;
procedure Foo(const s: string);
  begin
    proc;
    pbyte(s)^:=1; // force checkpointer call on s for testing purposes
  end;
begin
  setlength(s,10);
  Foo(s);
end.

If this is not enough
- we can introduce a compiler switch (or reuse an existing) which builds a checksum on the relevant data of a const parameter on function entry. At function exit, this checksum is checked.

That assumes are reproducible error, (and implied awareness that there is a bug at all). Both aer none-given.

A switch to disable the feature entirely may have it's own use. But it is entirely different to the proposal I made.
And (with the exception of a real tiny overlap) it does not solve the problems that my proposal tires to solve.

that is to produce an release executable with the optimization ENABLED, but previously test for *certain* errors in it.

Certain !!! Not all. No check however clever can catch all errors.

This is about finding screwups of the refcount. The data modification detection of strings is a side effect only.

A string variable (the actual variable containing the hidden pointer) in pascal should have one of 2 states:
- it could be nil => that is an empty string
- It could have a memory address stored.
In this case, the compiler makes sure, that at this memory address a valid string can be found.
( The memory is allocated, and a string with ref count 1 or greater is found there)

Except for fillchar, move (and such low level routines) the compiler offers no way, to write any code that intentionally breaks the above.
- Well yes, if you typecast to a none ref counted type, you ay somehow be able
- And yes, if you write to random memory, you might hit a string too, but that's not the only exception, where the compiler does not enforce the String nil => means string points to valid/allocated memory, with actual string present... Theonly place that is not enforced is:
"const s: string" param.

And other than fillchar, I would not consider this a low level function, for which the user should be required of having knowledge of the internals on ref counted types.

Of course, if the user keeps the implicit promise, not to change that string, including the global var, that was passed in => then that is fine. Only accidents happen.
(Same as for range checks, Array x[1..3] is a promise not to access x[4]. Yet it can happen)

Anyway, I repeat. I do not try to detect, if the string content was changed => that is a side effect

I try to catch issues with the refcount.

They do not happen, if the string is part of a const param record

Of course, as soon as there are optimizations that assume the data wasn't changed, then there will be more issues. With and without rec counting.
And then we may start another discussion, looking at hose cases....

for now: ref counting.

Florian Klämpfl wrote

May I propose a more generic solution: - with some switch (-gc?) a call to checkpointer is inkoved at the end of a procedure for const ref. counted types. Together with -gh this catches the common case:

Sorry I do not understand?

I assume the below is supposed to be the hidden / automatic check? (since the above was the unwarranted change to s)

But what does it test (besides from being an assignment?

Yes.

(since the above was the unwarranted change to s) But what does it test (besides from being an assignment?

This is only to demonstrate that checkpointer detects the error.

Martin wrote

This is about finding screwups of the refcount. The data modification detection of strings is a side effect only.

Why is a "screwup" of the ref. count a problem? It is only a problem if it gets 0 and the string is disposed?

Why not?

This ...

{$h+}
type
  tr = record
    s : string;
  end;
var
  r : tr;
procedure proc;
  begin
    r.s:='1234';
  end;
procedure Foo(const r : tr);
  begin
    proc;
    // r.s is invalid now ...
  end;
begin
  setlength(r.s,10);
  Foo(r);
end.

... hurts as well because r is passed by value because it is small.

Yes, that is what I meant by "screw up" (maybe not the best word, my apology)

A change of refcount is perfectly fine

GLobVar1 := GlobVar2;
Foo(GLobVar1); // const param

While in Foo:
- you can change GLobVar2 as much as you want, no problem, perfectly legal, breaks no rule.
- you can even change GLobVar1 (tecnically breaks the rule, but GLobVar2 keeps things ok)

- If you change both (and both in a way that memory moves), then there is a problem.
Because then "S" in foo, is a string, that is not nil, yet points to invalid (unallocated, or worse allocated by something else) memory. That violates the promise that a string type is correctly managed. (Ok, the implementor broke a promise first,, an eye for an eye, ...)

....

ok, yes, I overlooked the "passed by value" possibility.

Well, if those case can be checked too, great.
If, not, sad...

The amount of strings (and if they are in a const param by value or by ref) should be known at compile time.

Except for dyn-array (or array of const) => dyn array, means that the string is in as tructor that comes by ref. If the content of the dyn array changes, then it is reflected in Foo too)

And what exactly does checkpointer check?

That s is not nil ? => s will never be nil. If something goes wrong, s points to memory of which nothing is known (could be unallocated, could be allocated by something else)

Martin wrote

And what exactly does checkpointer check?

Check if the pointer points into a allocated memory block.

Combined with heaptrc.keep_released:=true, this is a very powerful check and even more if heaptrc is compiled with FPC_HEAPTRC_EXTRA. Downside is a slight slow down and a heavily increased memory consumption because keep_released:=true forces the heap manager never to reused a used memory block so checkpointer finds any dangling pointer. But I think it finds almost all pointer related problems.

With the default settings (keep_released:=false) it gets not all of them because a memory block might be already reallocated and the pointer might look valid.

I propose this because my standard procedure if something strange happens and I suspect heap corruption and pointer problemes:
- compile with -gh
- use keep_released:=true
- use a heaptrc unit compiled with FPC_HEAPTRC_EXTRA

That s is not nil ? => s will never be nil. If something goes wrong, s points to memory of which nothing is known (could be unallocated, could be allocated by something else)

Not with heaptrc.keep_released:=true

Florian Klämpfl wrote

Am 23.06.2011 23:03, schrieb Martin: > And what exactly does checkpointer check? Check if the pointer points into a allocated memory block.

Ok, that would actually work.

even without keep_released it could catch quite some

It should check, that it points to the correct place inside an allocated
block (That is String^ := AllocAddr + SizeOf(StringLen) + SizeOf(StrRefCnt)

It could even check, that the refcount is not zero. If the memory has not yet been reallocated, that should fail, if the string was released (assuming the last DecRefCnt, actually set's it to zero, before releasing the memory) (Would not work with -gt => since then all values are trashed)

It would definitely be a good start.

Another option would be to to the entire check, with a special memory manager.

Let the memory manager, add a byte to each request (probably 4 bytes for boundaries), and add a ref-counting like structure. This would only be used by "const param" => const param can use this to mark the block of memory (never mind if it is a string or elsewhat) as referenced => trying to free it, will cause an error

One could even add checksumming the memory...

Ok, probably to much...

entry of a procedure, and compare the parameters against these copies on exit. This will not modify the behaviour of the code, because the refcounts are not altered, and it will catch *all* modifications, that may have altered the strings.

Dunno whether and how such a feature should be implemented in the compiler - my suggestion can also be implemented in code, perhaps assisted by macros for easy on/off switching.

DoDi

Fetching content changes, would require to either copy or checksum *every* const param => that is a different beast (one that I did not propose...)

Content changes in structured types (TObject!) are not prevented by the "const" modifier.

While such a restriction is one of the many meanings of "const" in C++, and could be implemented in the same way in OPL, too, it would require so many changes to existing code, that I discourage every such attempt in OPL. Consider the proper separaration of const/non-const methods in every class, with possibly dual implementations of the same methods :-(

DoDi

Maybe just adding the option to disable the const optimization and letting the programmer figure out whether it fixes or not a bug is a good enough addition?

+1 :-)

DoDi