Waldek Hebisch wrote:
It looks that the problem is deeper. The following program fails both with and without your patch:
I see. (BTW, I think it's actually a different bug than the one I fixed, but it doesn't matter.)
AFAICS if the first parameter to `readstr' is not a plain variable,
In this case I think we only need to fix the reference, which save_expr_string already does. (E.g., functions returning strings use a temporary variable internally, and save_expr_string makes sure that the function is called only once, but the internal variable can be used several times.)
or if any other parameter may have side effects, then we need to copy the string parameter.
Yes. We should do a new_string_by_model then, I think. (The check for side-effects could be refined, of course, but I think it would already catch the majority of cases.)
For `Val', we either have to check the 2nd argument for side-effects and copy the string if so, or (perhaps better), force evaluation of the conversion (whose result is always a simple numeric type) before evaluation of the 2nd argument via a save_expr or temp variable. (I'm not sure if BP actually documents this as the standards do for `ReadStr', but it seems to behave like this.)
When passing string expressions as const parameters we create a temparary variable, but is is not clear if the temparary will live long enough
That's what I avoided with my patch.
(in fjf1099.pas the temporary is destroyed too early, but I do not see why other similar cases work).
I'm also not sure why other cases seemed to work, but I really didn't try too hard to break them as I already knew it was wrong.
However, I do not know what `StringTFDD_Reset' is supposed to do. ATM it looks that `StringTFDD_Reset' assume that the arguments remain valid when reading from the corresponding file. So, the natural specification can be: "Arguments to StringTFDD_Reset must be variables, must stay unmodified as long as ....". This specification is messy and the usage is error prone,
IMHO it's more or less analogous with files. One could argue about what happens when the length changes, but on files with read-buffering (which GPC does), reading can also be undefined when the external file is modified meanwhile.
but with this specification the fix would be to change the third parameter to StringTFDD_Reset into a var parameter.
That's why I did so.
OTOH if we want to promise more, then we need to think twice how we can implement this.
I'm not sure we should. The user can always make a more permanent copy himself when needed.
Frank