Recent

Author Topic: [SOLVED] Commit ee10850a of 17 Oct, 2021 breaks msegui compatibility.  (Read 7014 times)

Fred vS

  • Hero Member
  • *****
  • Posts: 2515
    • StrumPract is the musicians best friend
Hello.

    After high fight I found the commit that creates problems:
    Commit ee10850a of 17 Oct, 2021: Reducing and aligning ...
    https://gitlab.com/freepascal.org/fpc/source/-/commit/ee10850a5793b69b19dc82b9c28342bdd0018f2e

    Since that commit all my applications do not run anymore.

  This was the answer of Florian:

Quote
Well, they probably make assumptions about internla structures they shouldn't:

https://wiki.freepascal.org/User_Changes_Trunk#System_-_Ref._count_of_strings

Hum, who are "they probably make assumptions"?
The fpc committers or the libraries of my applications?

I am lost and dont see where to look.

Please and re-please would it be possible to do commits that does not break compatibility?
If no, where to look, what to fix, the "User_Changed..." and his "how to fix" did not help.

Thanks.

Fre;D
« Last Edit: October 31, 2021, 10:52:23 pm by Fred vS »
I use Lazarus 2.2.0 32/64 and FPC 3.2.2 32/64 on Debian 11 64 bit, Windows 10, Windows 7 32/64, Windows XP 32,  FreeBSD 64.
Widgetset: fpGUI, MSEgui, Win32, GTK2, Qt.

https://github.com/fredvs
https://gitlab.com/fredvs

Bart

  • Hero Member
  • *****
  • Posts: 4516
    • Bart en Mariska's Webstek
Re: Commit ee10850a of 17 Oct, 2021 breaks msegui compatibility.
« Reply #1 on: October 27, 2021, 05:38:16 pm »
Please and re-please would it be possible to do commits that does not break compatibility?

Well, this is going to happen in trunk (or should I say main) every now and then.

Bart

Jonas Maebe

  • Hero Member
  • *****
  • Posts: 922
Re: Commit ee10850a of 17 Oct, 2021 breaks msegui compatibility.
« Reply #2 on: October 27, 2021, 07:58:55 pm »
Hum, who are "they probably make assumptions"?
The fpc committers or the libraries of my applications?
Your application probably makes an assumption about the internal layout of the header that precedes ansistrings (tansirec in the system unit, rtl/inc/astrings.inc). This record type is not exported via the interface of the system unit, which means code outside the compiler/RTL cannot rely on its layout or contents (except via public helpers such as System.StringRefCount, as mentioned on the User Changes page).

Fred vS

  • Hero Member
  • *****
  • Posts: 2515
    • StrumPract is the musicians best friend
Re: Commit ee10850a of 17 Oct, 2021 breaks msegui compatibility.
« Reply #3 on: October 28, 2021, 03:37:23 pm »
Hum, who are "they probably make assumptions"?
The fpc committers or the libraries of my applications?
Your application probably makes an assumption about the internal layout of the header that precedes ansistrings (tansirec in the system unit, rtl/inc/astrings.inc). This record type is not exported via the interface of the system unit, which means code outside the compiler/RTL cannot rely on its layout or contents (except via public helpers such as System.StringRefCount, as mentioned on the User Changes page).

Hello Jonas and thanks for answer.

After lot of long hours of trying to fix, I have to stop, I cannot fix it.

Quote
except via public helpers such as System.StringRefCount
Sadly StringRefCount works only for Tansistring and Tunicodestrings.

But could it be possible to think to add condition in the problematic commit.
After deep tests it appears that the problems occur with the patch done to rtl/inc/astrings.inc and /rtl/inc/ustrings  ( patch for compiler/aasmcnst.pas is OK. )

So the proposition is this, use a define to give the possibility to user to not use the patch:

This is the problematic patch in rtl/inc/astrings.inc and /rtl/inc/ustrings :

Code: Pascal  [Select][+][-]
  1. {$if not defined(VER3_0) and not defined(VER3_2)}
  2.   {$ifdef CPU64}       
  3.     Ref         : Longint;
  4.   {$else}
  5.     Ref         : SizeInt;
  6.   {$endif}
  7. {$else}
  8.   {$ifdef CPU64}       
  9.     { align fields  }
  10.         Dummy       : DWord;
  11.   {$endif CPU64}
  12.     Ref         : SizeInt;
  13. {$endif}
  14.  

And replace by something like this (see no64bitRref):

Code: Pascal  [Select][+][-]
  1. {$if (not defined(VER3_0)) and (not defined(VER3_2)) and (not defined(no64bitRref))} // here change
  2.   {$ifdef CPU64}       
  3.     Ref         : Longint;
  4.   {$else}
  5.     Ref         : SizeInt;
  6.   {$endif}
  7. {$else}
  8.   {$ifdef CPU64}       
  9.     { align fields  }
  10.         Dummy       : DWord;
  11.   {$endif CPU64}
  12.     Ref         : SizeInt;
  13. {$endif}

With this, if somebody does not want the new feature, just add that parameter at compil : -dno64bitRref

And so everybody is happy, even the people that uses libraries that dont like the original commit.

Fre;D
« Last Edit: October 28, 2021, 03:53:42 pm by Fred vS »
I use Lazarus 2.2.0 32/64 and FPC 3.2.2 32/64 on Debian 11 64 bit, Windows 10, Windows 7 32/64, Windows XP 32,  FreeBSD 64.
Widgetset: fpGUI, MSEgui, Win32, GTK2, Qt.

https://github.com/fredvs
https://gitlab.com/fredvs

marcov

  • Administrator
  • Hero Member
  • *
  • Posts: 9712
  • FPC developer.
Re: Commit ee10850a of 17 Oct, 2021 breaks msegui compatibility.
« Reply #4 on: October 28, 2021, 03:48:27 pm »
So the proposition is this, use a define to give the possibility to user to not use the patch:

This is not how it works. We can't cater to faulty code. If you can't fix it, you simply have to stay
with older versions till you can fix the problem in your code.

Fred vS

  • Hero Member
  • *****
  • Posts: 2515
    • StrumPract is the musicians best friend
Re: Commit ee10850a of 17 Oct, 2021 breaks msegui compatibility.
« Reply #5 on: October 28, 2021, 03:56:14 pm »
So the proposition is this, use a define to give the possibility to user to not use the patch:

This is not how it works. We can't cater to faulty code. If you can't fix it, you simply have to stay
with older versions till you can fix the problem in your code.

Yes but the problem is not only in my code but in code of other libraries too.
I use Lazarus 2.2.0 32/64 and FPC 3.2.2 32/64 on Debian 11 64 bit, Windows 10, Windows 7 32/64, Windows XP 32,  FreeBSD 64.
Widgetset: fpGUI, MSEgui, Win32, GTK2, Qt.

https://github.com/fredvs
https://gitlab.com/fredvs

Jonas Maebe

  • Hero Member
  • *****
  • Posts: 922
Re: Commit ee10850a of 17 Oct, 2021 breaks msegui compatibility.
« Reply #6 on: October 28, 2021, 09:45:05 pm »
After deep tests it appears that the problems occur with the patch done to rtl/inc/astrings.inc and /rtl/inc/ustrings  ( patch for compiler/aasmcnst.pas is OK. )
If you revert the changes in the RTL, you also have to revert the compiler change. Otherwise the compiler will generate data for the new definition while the code in the RTL uses the old definition.

PascalDragon

  • Hero Member
  • *****
  • Posts: 3525
  • Compiler Developer
Re: Commit ee10850a of 17 Oct, 2021 breaks msegui compatibility.
« Reply #7 on: October 30, 2021, 03:22:11 pm »
Quote
except via public helpers such as System.StringRefCount
Sadly StringRefCount works only for Tansistring and Tunicodestrings.

First of they're named AnsiString and UnicodeString without any T-prefix and second these two types are the only two types that use the record so that StringRefCount only works with those two types is no problem.

And so everybody is happy, even the people that uses libraries that dont like the original commit.

No, that means that you can't use precompiled units from someone else and also - as Jonas pointed out - that you can even use the same compiler. This will be even more true once we support dynamic packages and officially ship libraries, because they will be precompiled as well.

Yes but the problem is not only in my code but in code of other libraries too.

No, the problem is relying on undocumented implementation details. We don't give guarantees that implementation details stay the same, because these details might be because of some bug that needs to be fixed.

Fred vS

  • Hero Member
  • *****
  • Posts: 2515
    • StrumPract is the musicians best friend
Re: Commit ee10850a of 17 Oct, 2021 breaks msegui compatibility.
« Reply #8 on: October 30, 2021, 04:42:53 pm »
Quote
except via public helpers such as System.StringRefCount
Sadly StringRefCount works only for Tansistring and Tunicodestrings.

First of they're named AnsiString and UnicodeString without any T-prefix and second these two types are the only two types that use the record so that StringRefCount only works with those two types is no problem.

Hello Sven.
Ok, but in comment they said:
Quote
patch by Sergey Larin: Reducing and aligning the size of TAnsiRec, TUnicodeRec for CPU64


Yes but the problem is not only in my code but in code of other libraries too.
No, the problem is relying on undocumented implementation details. We don't give guarantees that implementation details stay the same, because these details might be because of some bug that needs to be fixed.

Sorry but I dont understand this "No, the problem is relying on undocumented implementation details.".
What are the "implementation details", and what bug must be fixed ?
Do you want to say that there was bug in the libraries used before the commit ee10850a?
Strange because all was functioning perfectly before that commit.

Now about how to fix the bug after the commit ee10850a.

I did spent some (many) time to fix it but without luck.

Does somebody have a idea what could affect that difference in the reference counter of AnsiString and UnicodeString ?
What method will have impact on that change and what must be changed?

Thanks.

Fre;D

I use Lazarus 2.2.0 32/64 and FPC 3.2.2 32/64 on Debian 11 64 bit, Windows 10, Windows 7 32/64, Windows XP 32,  FreeBSD 64.
Widgetset: fpGUI, MSEgui, Win32, GTK2, Qt.

https://github.com/fredvs
https://gitlab.com/fredvs

Jonas Maebe

  • Hero Member
  • *****
  • Posts: 922
Re: Commit ee10850a of 17 Oct, 2021 breaks msegui compatibility.
« Reply #9 on: October 30, 2021, 05:34:22 pm »
Does somebody have a idea what could affect that difference in the reference counter of AnsiString and UnicodeString ?
The reason nobody really has any idea about this or can't tell you this, is because this change cannot affect any code that uses ansistrings/unicodestrings "normally": concatenating, passing as parameters, returning as function result, using them as a pchar, converting them to other string types, ...

It can only cause problems if MSE somewhere e.g. typecasts an ansistring or unicodestring to a pointer, and then uses pointer arithmetic to manually calculate what it thinks is the offset of the reference count (or another field whose offset changed). That's also why we say this change can only break code that relies on an implementation detail. There was/is no guarantee about how the header that sits in front of an ansistring or unicodestring looks like, which is also the reason why it could be changed.

Fred vS

  • Hero Member
  • *****
  • Posts: 2515
    • StrumPract is the musicians best friend
Re: Commit ee10850a of 17 Oct, 2021 breaks msegui compatibility.
« Reply #10 on: October 30, 2021, 05:53:36 pm »
Does somebody have a idea what could affect that difference in the reference counter of AnsiString and UnicodeString ?
It can only cause problems if MSE somewhere e.g. typecasts an ansistring or unicodestring to a pointer, and then uses pointer arithmetic to manually calculate what it thinks is the offset of the reference count (or another field whose offset changed).

Hello Jonas.

Many thanks for that precious infos.
Yes, this is exactly the problem that I wanted to explain.

And it is exactly what I dont find: where, in MSE code, they typecast an ansistring or unicodestring to a pointer wrongly.
Anyway, I know now precisely where to look (again).

Thanks for helping.

Fre;D


I use Lazarus 2.2.0 32/64 and FPC 3.2.2 32/64 on Debian 11 64 bit, Windows 10, Windows 7 32/64, Windows XP 32,  FreeBSD 64.
Widgetset: fpGUI, MSEgui, Win32, GTK2, Qt.

https://github.com/fredvs
https://gitlab.com/fredvs

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 7539
  • Debugger - SynEdit - and more
    • wiki
Re: Commit ee10850a of 17 Oct, 2021 breaks msegui compatibility.
« Reply #11 on: October 30, 2021, 06:26:47 pm »
Your code probably casts an (ansi-)string to a pointer, and accesses memory in front of the pointer.

Something like:
Code: Pascal  [Select][+][-]
  1.   if PLongInt(MySomeString)[-1] = 0 then  

Such code assumes that it finds the length or refcount or codepage of the string at the address accessed by the expression.

But, the length/refcnt/codepage is no longer there. It was just random coincidence that it used to be there. It's called "implementation detail", and it can always change with no warning at all.


FPK

  • Moderator
  • Full Member
  • *****
  • Posts: 115
Re: Commit ee10850a of 17 Oct, 2021 breaks msegui compatibility.
« Reply #12 on: October 30, 2021, 07:37:46 pm »
I suppose Martin would have appreciated this change as it reduces memory usage and efficiency with little to no drawback.

Roland57

  • Full Member
  • ***
  • Posts: 214
Re: Commit ee10850a of 17 Oct, 2021 breaks msegui compatibility.
« Reply #13 on: October 31, 2021, 07:35:14 am »
Your code probably casts an (ansi-)string to a pointer, and accesses memory in front of the pointer.

Something like:
Code: Pascal  [Select][+][-]
  1.   if PLongInt(MySomeString)[-1] = 0 then  

Such code assumes that it finds the length or refcount or codepage of the string at the address accessed by the expression.

For example this procedure (in kernel/msestrings.pas)?
Code: Pascal  [Select][+][-]
  1. procedure stringaddref(var str: ansistring);
  2. var
  3.  po1: psizeint;
  4. begin
  5.  if pointer(str) <> nil then begin
  6.   po1:= pointer(str)-2*sizeof(sizeint);
  7.   if po1^ >= 0 then begin
  8.    inc(po1^);
  9.   end;
  10.  end;
  11. end;

Jonas Maebe

  • Hero Member
  • *****
  • Posts: 922
Re: Commit ee10850a of 17 Oct, 2021 breaks msegui compatibility.
« Reply #14 on: October 31, 2021, 08:50:40 am »
For example this procedure (in kernel/msestrings.pas)?
Yes, absolutely. Here's an alternative way to implement that hack. It's still quite iffy, but at least a bit safer, and compatible both with old and new FPC versions:
Code: [Select]
    procedure stringaddref(var str: ansistring);
    var
     temp: ansistring;
    begin
     { increase refcount if it was > 0}
     temp:=str;
     { prevent the compiler from finalising temp, so it won't decrease the refcount again }
     pointer(temp):=nil;
    end;

 

TinyPortal © 2005-2018