Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes for 64bit Windows and Delphi 10.2/10.3 #44

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fixes for 64bit Windows and Delphi 10.2/10.3 #44

wants to merge 3 commits into from

Conversation

deanm-gp
Copy link

@deanm-gp deanm-gp commented Feb 1, 2019

-changed procedure GDIError to use Length instead of SizeOf so it work correctly when Char is WideChar
-fixed JclDebug to work correctly when 64bit system has more than 4GB memory
-fixed to work correctly in Delphi 10.2/10.3

var
I: Integer;
begin
I := IndexOfSegment(Addr);
Result := DWORD(-1);
Result := High(TJclAddr);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct or should this be Low(TJclAddr)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low(TJclAddr) SHOULD be 0 (as it is in 32bit and FPC mode) so High(TJclAddr) is correct but I seem to have missed a change in JclBase.pas.
Line:
TJclAddr64 = Int64;
should be replaced with.
TJclAddr64 = UInt64;

To support older Delphi maybe something like this would be better:
{$IF declared(UInt64)}
TJclAddr64 = UInt64;
{$ELSE}
TJclAddr64 = Int64;
{$ENDIF}
but I don't have the old Delphi to test this with.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TJclAddr64 should not be changed to unsigned, it will break all memory operations that result in negative offsets which are valid right now.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, that sounds like a design fault, as memory addresses are never signed, can never be negative. Offsets can, however, so perhaps those memory operations should be updated to work with offsets? (And be annotated that offsets are signed, so can't be used to indicate absolute memory addresses.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if they could all be found inside the JCL, it would be impossible to find them in user's code and it is really hard to track down a bug resulting from signed/unsigned mixup.
So no, this will have to stay this way.
As to the original cast, I would simply keep the -1 and cast it to TJclAddr instead of DWORD.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deprecated warnings can help spot the use of obsolete symbols in user-code. But I agree with the "no breaking changes" approach.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TJclAddr64 is already unsigned in FPC and TJclAddr32 is unsigned in 32 bit code. If 32 bit code already works with unsigned addresses then 64 bit code should too.
64 bit code definitely does not work with what is in JclDebug if you have more than 4GB RAM and code that I have committed has worked in production for over a year.
Even if you keep TJclAddr64 as signed, the rest of the code should still work for the next few decades until OS and hardware can hit the top of the 64 bit address space but what is there before this change does not work on most of the modern computers.
As far as splitting the commit, I don't think it's required, just use a decent difference viewer and the reason for each change becomes obvious.
Most of the changes in the commit were made to fix the 64 bit code which used Integer/DWORD to cast addresses and references. There were few bits were changes made fixed the Ansi to Unicode strings, mostly by changing SizeOf to Length.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems toe pull request is not 100% approved. There seem to be some things in it though, which are not disputed. Can they be put into a separate pull request? Or if they affect different topics maybe several pull requests?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the commits contained in this pull request was singled out and pulled in now:
JclGraphUtils GDIError: replaced SizeOf by Length #53
another one as well where I put the first part of this part of this pull request here in:
370cc0b

Now to my point: I didn't (yet) create a pull request for the 2nd part of that one, as the Rio IDE constantly puts System.ImageList before the {$IFDEF HAS_UNITSCOPE} in the line this request wants to change and I also no not know why this unit is necessary, as ImgList is in the uses clause already. Means: I didn't fully understand what the requester wants to achieve with this change.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About 64 memory addresses never reaching the sign-bit: that's not true. Consider top-down allocation for example, or random address executable location, or loading a 64 bit DLL with a fixed load address that has the sign bit set

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants