View Issue Details

IDProjectCategoryView StatusLast Update
0035753FPCCompilerpublic2019-06-23 16:13
ReporterAkira1364Assigned ToJonas Maebe 
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Platformx86_64OSWindowsOS Version10
Product Version3.3.1Product Build42272 
Target VersionFixed in Version3.3.1 
Summary0035753: Delphi allows dereferencing a pointer to a [0..0] static array and accessing it with indices > 0, however 0035671 breaks this.
DescriptionPlease note: Personally, I think the particular pattern I'm describing here is both stupid and nonsensical, and I've never been particularly sure why anyone would write something that way, however I thought I'd point this out as there's (unfortunately IMO) a lot of code out there that does it.

That said, what I mean is, in Delphi, the following example compiles fine:

program Example;

{$ifdef FPC}
  {$mode Delphi}
  uses SysUtils;
{$else}
  {$AppType Console}
  uses System.SysUtils;
{$endif}

type
  TMyPoint = record
    X: Single;
    Y: Single;
  end;
  TMyPoints = array[0..0] of TMyPoint;
  PMyPoints = ^TMyPoints;

var
  P: PMyPoints;
  I: NativeUInt;

begin
  GetMem(P, 5 * SizeOf(TMyPoint));
  for I := 0 to 4 do begin
    with P^[I] do begin
      X := 12.0;
      Y := 24.0;
    end;
  end;
  for I := 0 to 4 do begin
    with P^[I] do
      WriteLn(Format('[X: %f, Y: %f]', [X, Y]));
  end;
  FreeMem(P, 5 * SizeOf(TMyPoint));
end.

Again, it clearly does not really *make sense* that this works, because by doing "P^[I]" you are *explicitly* accessing a static array that *explicitly* does not have valid indices higher than zero, but for reasons that might not even be intentional, Delphi has always allowed this and does not even warn about it.

FPC has *always* at least warned about it (as it should!) but after 0035671 now unavoidably errors out in {$mode Delphi}.

Personally I'm not concerned if the FPC team does not want to change this behavior further, as I think it's good to discourage writing that kind of thing in the first place (and I'd even honestly like to see {$mode ObjFPC} error out on it as well!), however, there are probably some people out there who will complain, so like I said I figured I would point it out.
Steps To ReproduceCompile the provided example program.
TagsNo tags attached.
Fixed in Revision42275
FPCOldBugId
FPCTarget-
Attached Files

Relationships

related to 0016006 closedJonas Maebe Dead code mistake 
related to 0035671 closedJonas Maebe Enable range check for Delphi-mode 

Activities

Akira1364

2019-06-22 21:31

reporter   ~0116856

Last edited: 2019-06-22 22:40

View 4 revisions

One other thing, which I meant to mention, is that, in general, anyone who *does* have code written like:

type
  TMyPoints = array[0..0] of TMyPoint;
  PMyPoints = ^TMyPoints;

should almost certainly just change it to:

type
  PMyPoint = ^TMyPoint;

There's literally no logical reason to declare a one-item static array type separately, and then a pointer type alias that points to that.

Just use a pointer type alias that points *directly* to the underlying type (which you can access like an array just fine, with no explicit dereferencing needed.)

Jonas Maebe

2019-06-22 21:46

manager   ~0116858

Last edited: 2019-06-22 22:11

View 4 revisions

FPC now gives an error because the resulting code has always had undefined behaviour in FPC, i.e., it was unpredictable how it would behave. The reason is that unlike Delphi, FPC is and always has been strict about subrange types: FPC always generates code based on the assumption that a subrange/enumeration variable contains a value that is valid for the declared range (just like that an ansistring contains a valid ansistring pointer, or that a dereferenced pointer contains a valid address). Unfortunately, it is not possible to make this code generation depend on the syntax mode: either it's like that everywhere, or nowhere (see the first link below for the reason).

It seemed better to give a compile-time error than to generate code that may behave differently than expected, especially when using non-constant indices. A Delphi-compatible way to fix this kind of code is to declare the array as an array of the maximum possible number of elements rather than of one element. I understand this is annoying when porting/sharing code, but between a compile-time error and getting code that will potentially silently misbehave (like before this change), the new behaviour seemed to be the less bad one.

See also these related threads and bug reports:
* https://forum.lazarus.freepascal.org/index.php/topic,45507.msg322059.html#msg322059
* 0016006 and related issues
* https://forum.lazarus.freepascal.org/index.php/topic,44655.msg314221.html

Akira1364

2019-06-22 22:36

reporter   ~0116860

Last edited: 2019-06-22 22:39

View 2 revisions

Again, I'm not really *personally* concerned about this.

I even just rebuilt Lazarus and did encounter a few problems with Delphi-mode components caused by the change, but in all cases simply using a pointer to the type instead of a pointer to an array[0..0] of the type fixed the issue.

J. Gareth Moreton

2019-06-23 00:27

developer   ~0116861

Just to note, the Windows unit has some UGLY constructs such as LOGPALETTE:

LOGPALETTE = record
  palVersion : WORD;
  palNumEntries : WORD;
  palPalEntry : array[0..0] of PALETTEENTRY;
end;

I remember this existing way back in the days of Delphi 2.0 - the reason why it is built this way is because you would pass a Pointer to a LOGPALETTE structure to API functions such as CreatePalette(), where the first few bytes contain a header (palVersion and palNumEntries) and the data that follows is a quartet of bytes for each palette entry. If 0035671 is to remain, records such as this will have to be addressed without breaking the interface to the Windows API. (And actually, in FPC 3.0.4, the unit uses a var parameter for CreatePalette(), which makes it very difficult to set the palette data.)

jamie philbrook

2019-06-23 00:42

reporter   ~0116862

Windows constructs are not the only place where this takes place. It is very common to have variable trailing data on
records and members of the record manage it.

 I normally use the $PUSH, $R-, do the code and then $POP to avoid any range checking..

 I know my opinion does not hold much weight but personally If this was done to save the coder from themselves then
maybe they should step back and take a real long look at what they are doing. I thought the Range Check feature was
designed for this and thus be able to turn it off in areas where you, the code, knows better to save some extra steps in
coding and simply have the compiler let you do what you want?

Akira1364

2019-06-23 05:54

reporter   ~0116864

Last edited: 2019-06-23 05:55

View 2 revisions

I'm not sure that either of the last two points are all that relevant. We're talking about *pointers* here.

To put it very simply, if you have a type defined like:

TSomethings = array[0..0] of TSomething
PSomethings = ^TSomething

and then you go and do something like:

GetMem(MyPSomethingsVar, 5 * SizeOf(TSomething));
WriteLn(MyPSomethingsVar^[2].SomeField);

you are *explicitly, blatatantly* accessing an array index that simply does not exist. For no reason whatsoever. You should have instead written this:

PSomething = ^TSomething

and then this:

GetMem(MyPSomethingVar, 5 * SizeOf(TSomething));
WriteLn(MyPSomethingsVar[2].SomeField);

Where the pointer variable is just a direct *pointer to the underlying type*, which can validly be assigned any length with GetMem, and accessed in an array-like fashion *without* the caret dereference operator being present at any point.

Basically, stuff like:

TSomethings = array[0..0] of TSomething

serves no logical purpose of any kind, and never has.

J. Gareth Moreton

2019-06-23 06:18

developer   ~0116865

Last edited: 2019-06-23 06:26

View 2 revisions

You missed the point with the LOGPALETTE definition. It's explicitly defined that way in the Windows units - you pass a pointer to a LOGPALETTE into CreatePalette(), but the data block is larger than the size of LOGPALETTE because you have to do things like "Palette^.palPalEntry[$FF].peRed := $FF;" to modify the individual palette entries before you pass your data block into the function. The size of the data block is stored in the first four bytes of said data block.

To make it extra awkward, you have to do something like "GetMem(Palette, SizeOf(LOGPALETTE) - ((PaletteEntryCount - 1) * SizeOf(PALETTEENTRY)));", subtracting one from the number of colours you wish to include because SizeOf(LOGPALETTE) includes the lone PALETTEENTRY.

What I'm saying is that if you wish to block this awkwardness, you need an alternative so the Windows API can still be supported.

Jonas Maebe

2019-06-23 09:19

manager   ~0116866

That is a good point, Gareth. While the Windows API issue could theoretically be solved with something like OffsetOf (0015416), that would making porting a lot more awkward. Maybe we should add an exception for [0..0] array declarations, or array[0..x] declarations in general (a bit similar to how pointers to Y can be implicitly converted to "array[0..x] of Y", but not to other array types.

J. Gareth Moreton

2019-06-23 12:04

developer   ~0116868

I did wonder if having an explicit "unbounded" array akin to "0..x", although since x is frequently a variable name, we might need to go for something else such as "array[] of T". Other things like what SizeOf returns and what happens if you try to declare a variable of that type rather than a pointer would need to be worked out too, or if an approach can be made so pointers aren't required and is hence safer and more portable.

This is assuming a new language feature is even desired.

Jonas Maebe

2019-06-23 14:07

manager   ~0116870

I meant "x" as placeholder for "a number", rather than as literal "x".

For now, I am changing it back to a warning.

Issue History

Date Modified Username Field Change
2019-06-22 21:10 Akira1364 New Issue
2019-06-22 21:31 Akira1364 Note Added: 0116856
2019-06-22 21:31 Akira1364 Note Edited: 0116856 View Revisions
2019-06-22 21:46 Jonas Maebe Note Added: 0116858
2019-06-22 21:47 Jonas Maebe Note Edited: 0116858 View Revisions
2019-06-22 21:52 Jonas Maebe Relationship added related to 0016006
2019-06-22 21:52 Jonas Maebe Relationship added related to 0035671
2019-06-22 22:04 Jonas Maebe Note Edited: 0116858 View Revisions
2019-06-22 22:11 Jonas Maebe Note Edited: 0116858 View Revisions
2019-06-22 22:36 Akira1364 Note Added: 0116860
2019-06-22 22:39 Akira1364 Note Edited: 0116860 View Revisions
2019-06-22 22:39 Akira1364 Note Edited: 0116856 View Revisions
2019-06-22 22:40 Akira1364 Note Edited: 0116856 View Revisions
2019-06-23 00:27 J. Gareth Moreton Note Added: 0116861
2019-06-23 00:42 jamie philbrook Note Added: 0116862
2019-06-23 05:54 Akira1364 Note Added: 0116864
2019-06-23 05:55 Akira1364 Note Edited: 0116864 View Revisions
2019-06-23 06:18 J. Gareth Moreton Note Added: 0116865
2019-06-23 06:26 J. Gareth Moreton Note Edited: 0116865 View Revisions
2019-06-23 09:19 Jonas Maebe Note Added: 0116866
2019-06-23 12:04 J. Gareth Moreton Note Added: 0116868
2019-06-23 14:07 Jonas Maebe Note Added: 0116870
2019-06-23 16:12 Jonas Maebe Assigned To => Jonas Maebe
2019-06-23 16:12 Jonas Maebe Status new => resolved
2019-06-23 16:12 Jonas Maebe Resolution open => fixed
2019-06-23 16:12 Jonas Maebe FPCTarget => -
2019-06-23 16:13 Jonas Maebe Fixed in Version => 3.3.1
2019-06-23 16:13 Jonas Maebe Fixed in Revision => 42275