Hi,

thanks Sven, finally we have initial RTTI.pas version on trunk. Let me start with first serious issue and eventually patch:

function IsManaged(TypeInfo: PTypeInfo): boolean;

IsManaged can't work with records because we need know managed fields count (in Delphi when ManagedFldCount is bigger than 0 it means that record has managed fields and problem is solved). Two possible (and IMO correct) solutions:

1. I think it's time to new position in "User Changes" wiki page. We can fix that by new behavior (a more Delphi compatible, more logical, more proper and a little incompatible with old code, but the risk is minimal): ManagedFldCount should be fixed/corrected as mentioned in http://bugs.freepascal.org/view.php?id=29767 (so it should count *real* managed field as the name "ManagedFldCount" suggests. Otherwise we can't implement this function. Btw. old behavior for ManagedFldCount will be not usefully anymore with RTTI module/improved TypInfo for counting all fields.

2. http://bugs.freepascal.org/view.php?id=30687#c96093 PART 2 of reworked patch for Management Operators should be accepted. With this path we have access to Init RTTI table. Anyway solution 1 is strongly recommended.

Other "solutions" (IMO totally wrong):

3. New compiler intrinsic routine:

function IsManagedType(T: Type): Boolean;

which is no real alternative because IsManaged from RTTI.pas is more run-time dedicated and is impossible to replace IsManaged by IsManagedType for all scenarios. Generally new intrinsic IsManagedType is not wrong but in our context is useless...

4. New additional RTTI generated for records, for example new field for TTypeData:

ThisIsRealAndNotFakeManagedFldCount: Integer;

with additional list declared after ManagedFields: array[1..ManagedFldCount] of TManagedField:

ThisIsRealAndNotFakeManagedFields:
array[1..ThisIsRealAndNotFakeManagedFldCount] of TRealAndNotFakeManagedField

Or some flag in TTypeData for records:

YesIamManagedRecordAndIHaveRealManagedFieldsAndYesPleaseIgnoreManagedFldCountWhichIsLying:
boolean;

5. Super slowly checking for each field in ManagedFields, and eventually new checking for each record in ManagedFields... I am so sad that I need to post this point :\

We rename ManagedFldCount to TotalFieldCount, add a field ManagedFieldCount and a property ManagedFldCount that returns TotalFieldCount for backwards compatibility (and maybe marked as deprecated).

Regards,
Sven

10.12.2016 00:54 "Sven Barth":

We rename ManagedFldCount to TotalFieldCount, add a field ManagedFieldCount and a property ManagedFldCount that returns TotalFieldCount for backwards compatibility (and maybe marked as deprecated).

Looks almost like point 4. We need also to adjust ManagedFields. ManagedFieldCount
is not enough. User still is unable to figure which fields are managed without many additional conditions -,- . Delphi layout:

ManagedFields: array[0..ManagedFldCnt - 1] of TManagedField;
        NumOps: Byte;
        RecOps: array[1..NumOps] of Pointer;
        RecFldCnt: Integer;
        RecFields: array[1..RecFldCnt] of TRecordTypeField;
        RecAttrData: TAttrData;
        RecMethCnt: Word;
        RecMeths: array[1..RecMethCnt] of TRecordTypeMethod

In this context TotalFieldCount looks like yet another incompatibility.

2016-12-10 0:54 GMT+01:00 Sven Barth :

I've created new bug report to express my strong stand about this problem:

http://bugs.freepascal.org/view.php?id=31102
(copied from bug report)

FPC has long standing critical issue. ManagedFldCount field in TypInfo (TTypeData) contains "count" for all fields (but should contains "count" for managed fields only - as name suggest). It is not feature but highly visible bug.

* The bug has critical meaning for development of RTTI.pas because is impossible to implement in rational way IsManaged function (ManagedFldCount is used for that)
* The bug has many performance issues especially for InitializeArray/FinalizeArray.
* The bug means more Delphi incompatibility RTTI.
* For scripting languages it means many troubles (for Delphi porting code)
* TManagedField entry for each (I mean here unmanaged fields) field has zero value for programmer, it has no sense.

any partial fix with pseudo backward compatibility has no sens and will bring much more noise and confusion and more complicated RTTI code without real gain.

the proper layout for part related to records (in current state) in TTypeData is:

      tkRecord: ( // could be extended for info about attributes, methods
and operators (maybe also properties)
        RecSize: Integer;
        ManagedFldCount: Integer;
       {ManagedFields: array[0..ManagedFldCnt - 1] of TManagedField;
        RecFldCnt: Integer;
        RecFields: array[1..RecFldCnt] of TRecordTypeField});

where

  TRecordTypeField = packed record
    Field: TManagedField;
    Flags: Byte; // visibility
    Name: ShortString;
  end;

When we have additional "Name" and "Flag" for managed and unamanaged fields it has sense, because when we have more info than pure TManagedField for each field, it could be used for ORM or for scripts. Probably is good idea to correlate RecFields together with:

{$RTTI EXPLICIT METHODS([]) PROPERTIES([]) FIELDS([])}

The patch provided in #29767 is wrong when we thinking about extended RTTI (init table should stay simple as possible), same for MO PART 2 ( http://bugs.freepascal.org/view.php?id=30687#c96093 )

It must be changed same as was needed change for indirect type info (PPTypeInfo) for greater good.

2016-12-11 13:18 GMT+01:00 Maciej Izak :

http://bugs.freepascal.org/view.php?id=31102

I can provide proper patch for #31102 and for #31108 (for fields part).

TotalFieldCount seems redundant (finally we have in conception RecFldCnt), we could point deprecated ManagedFldCount property to RecFldCnt. New ManagedFieldCount might be compromise solution. Anyway I have feeling that keeping both : ManagedFldCount and ManagedFieldCount is very bad idea (especially with new layout for ManagedFields which must contains managed fields only).

Could you do that please? :-)

2016-12-12 13:07 GMT+01:00 silvioprog :

Could you do that please? :-)

Status of #31102 is unclear. I need clear response about ManagedFldCount to decide how to do it - as part of FPC or NewPascal or some mixed way to keep compatibility as much as possible (in NewPascal for sure I will correct ManagedFldCount, current value of ManagedFldCount and form of ManagedFields is unacceptable).

2016-12-12 13:32 GMT+01:00 Maciej Izak :

My more complex proposition from point 2. Full backward compatible... Fair enough, ready for Management Operators + fix for Initialize/FinalizeArray:

=== code begin ===
type
  PPRecInitTable = ^PRecInitTable;
  PRecInitTable = TRecInitTable;
  TRecInitTable = packed record
    Size: Longint;
    Terminator: Pointer;
    ManagedFieldCount: Longint;
    { ManagedFields: array[0..ManagedFieldCount - 1] of TManagedField; }
  end;
  TRecordTypeField = packed record
    Field: TManagedField; // :\ a little redundant info (we have this info
also in TotalFields, but we need to deal with $RTTI EXPLICIT)
    Flags: Byte; // visibility
    Name: ShortString;
  end;
  TTypeData =
...
        { tkRecord }
        property ManagedFldCount: Integer read GetManagedFldCount;
 deprecated 'Use TotalFieldCount'; // return TotalFieldCount
...
     tkRecord: (
        RecSize: Integer;
        RecInitTable: PPRecInitTable; // here is accessible
ManagedFieldCount and ManagedFields
        TotalFieldCount: Integer;
       {TotalFields: array[0..TotalFieldCount - 1] of TManagedField;
        RecFldCnt: Integer; // for $RTTI EXPLICIT ... FIELDS
        RecFields: array[0..RecFldCnt - 1] of TRecordTypeField});
=== code end ===

So merge for PART 2 of Management Operators might be not bad idea:

https://github.com/maciej-izak/freepascal/commit/ea23ca80630fae488990dcd4bc62ddc94b18d304

Now I feel that all of my ideas to solve this are expressed.

2016-12-12 15:53 GMT+01:00 Maciej Izak :

Field: TManagedField; // :\ a little redundant info (we have this info also in TotalFields, but we need to deal with $RTTI EXPLICIT)

To save memory I think we can use FieldIndex: Integer (instead of Field: TManagedField). FieldIndex is index to TotalFields item...

So, my own idea for this.

Keep the following points in mind however:
- reasonable source(!) backwards compatibility (see for example what I did when I switched from PTypeInfo to PPTypeInfo: the fields were changed to PPTypeInfo, but properties were added that return PTypeInfo; this means that code using e.g. ParentInfo will continue to work, but code that uses the address to ParentInfo will not; also code that writes to ParentInfo will fail (as did Lazarus for example))
- the INIT RTTI is left as is; it only contains the managed fields already and should be as compact as possible as it has to be written for every record while the FULL RTTI is only written for records that need it (used by TypeInfo(), used as published property (ToDo)) [Note: I'm aware that extended RTTI will influence that due to the typelist, but even then the availability can still be controlled by the user (think $weaklinkrtti for example)]

=== code begin ===

type
  TRecordField = record
    Visibility: TVisibility;
    TypeRef: PPTypeInfo;
    FldOffset: Integer;
    Name: PShortString; { can be NULL if no ext RTTI }
  end;
  TManagedField = TRecordField deprecated;

TTypeData = record

  // ...
    tkRecord: (
      RecSize: Integer;
      { maybe if needed: RecInitTable: Pointer; can still be added later

on }

      ManagedFieldCount: Integer;
      ManagedFieldTable: PRecordField; // points to first entry of

ManagedFields

      case Boolean of
        True: (ManagedFldCount: Integer deprecated 'Use

ManagedFieldCount or TotalFieldCount depending on your use case') False: (TotalFieldCount: Integer)
{ Fields: array[1..TotalFieldCount] of TRecordField } { ManagedFields: array[1..ManagedFieldCount] of PRecordField } // points to corresponding entry in Fields
{ Names: array[1..TotalFieldCount] of {packed} ShortString }

    );
  end;

=== code end ===

This allows all fields to be enumerated as usual (namely by getting a pointer to the area after ManagedFldCount/TotalFieldCount and indexing TRecordField/TManagedField there.
ManagedFieldTable allows a fast access to a table of pointers to all managed fields (considering that RTTI is usually used in more processing intensive contexts (e.g. form instantiation, scripting, invoke, etc) the additional indirection is acceptable.
Additionally there is the optional name table which is referenced from each field.

To ease access we could add properties that do the heavy lifting, though I'd move all that to a new type TRecordData which is referenced from TTypeData inside a case branch as RecordData: TRecordData with RecSize, ManagedFieldCount, ManagedFieldTable and the
ManagedFldCount/TotalFieldCount case in the other branch.

Regards,
Sven

2016-12-12 20:41 GMT+01:00 Sven Barth :

So, my own idea for this.

First insight looks good (kudos for union with "deprecated field" ^^) and we don't have redundant info, but sorry it can't work in presented form.

With your changes INIT RTTI is incompatible with RTTI, which is critical defect for RTL. FPC_(INITIALIZE|FINALIZE|COPY|ADDREF) (rtl/inc/rtti.inc) is used in many contexts - both for internal usage (where generally is used INIT RTTI) and for external usage (with regular RTTI). Since we have InitializeArray/FinalizeArray/CopyArray is possible to execute FPC_(INITIALIZE|FINALIZE|COPY) with RTTI for records (instead of INIT RTTI for records) which means AV.

To solve this issue you need obligatory to add RecInitTable: PPointer; (and Terminator for INIT RTTI). To move forward presented patch is obligatory:

https://github.com/maciej-izak/freepascal/commit/ea23ca80630fae488990dcd4bc62ddc94b18d304 *PART 2* http://bugs.freepascal.org/view.php?id=30687#c96093

This is first step. With second iteration we could add your modifications, I have no other objections. See above patch - it is already prepared for differences between INIT RTTI (TRecordInfoInit) and FULL RTTI (TRecordInfoFull)

Finally we can merge both ideas. User has access to:

* Classical array of compact "managed field" by RecInitTable
* All fields
* All managed fields (in your solution it has sense)

2016-12-12 23:14 GMT+01:00 Maciej Izak :

With second iteration we could add your modifications, I have no other objections.

New day new ideas :)

With my patch we don't really need ManagedFieldCount, ManagedFields, ManagedFieldTable in TTypeData (each of Managed* can still be added later). It means also much better memory usage and it solves RTL problems (which was mentioned in my previous message). All what we need is:

=== code begin ===
type
  TInitManagedField = record
    TypeRefRef: TypeInfoPtr;
    FldOffset: SizeInt;
  end;
  PPRecInitTable = ^PRecInitTable;
  PRecInitTable = TRecInitTable;
  TRecInitTable = packed record
    Size: Longint;
    Terminator: Pointer;
    ManagedFieldCount: Longint;
    { ManagedFields: array[0..ManagedFieldCount - 1] of TInitManagedField
; }
  end;
  TRecordField = record
    Visibility: TVisibility;
    TypeRef: TypeInfoPtr;
    FldOffset: SizeInt;
    Name: PShortString; { can be NULL if no ext RTTI }
  end;
  TManagedField = TRecordField deprecated;

TTypeData = record

  // ...
    tkRecord: (
      RecSize: Integer;
      RecInitTable: PPRecInitTable;
      case Boolean of
        True: (ManagedFldCount: Integer deprecated 'Use

RecInitTable^^.ManagedFieldCount
or TotalFieldCount depending on your use case')
False: (TotalFieldCount: Integer)
{ Fields: array[1..TotalFieldCount] of TRecordField } { Names: array[1..TotalFieldCount] of {packed} ShortString }

    );
  end;

=== code end ===

I have hope that this is final solution :)

objections.

Comment regarding your previous mail: right, I forgot that due to InitializeArray() and FinalizeArray() being publicly available version of the internal helpers we either need to keep INIT and FULL RTTI in sync or a way to differentiate them... That's why you added the Terminator field I guess?

New day new ideas :) With my patch we don't really need ManagedFieldCount, ManagedFields,

ManagedFieldTable in TTypeData (each of Managed* can still be added later). It means also much better memory usage and it solves RTL problems (which was mentioned in my previous message). All what we need is:

of TInitManagedField ; }

'Use RecInitTable^^.ManagedFieldCount or TotalFieldCount depending on your use case')

Okay, I think we can indeed go with this aside from one small remark: the double indirection for Pointers is only needed if data resides in different units as those might be in different dynamic packages. Since the INIT and FULL RTTI of a record are always in the same unit a single indirection is sufficient (thus PRecInitTable instead of PPRecInitTable).

Other than that I think we've found a good compromise for our wishes and hopes for this :)

Regards,
Sven

2016-12-13 11:09 GMT+01:00 Sven Barth :

Exactly.

IIRC something is (was?) wrong (AV) for Mac i386 and Win64 when is used PRecInitTable instead of (indirect) PPRecInitTable... It is strange because for example for Win32 and Aarch64 PRecInitTable works fine... Have you an idea?

Other than that I think we've found a good compromise for our wishes and hopes for this :)

Phew! So... Please merge PART 2 of MO :P after this step I can start work on RTTI.pas and on extended info for records (with small "baby steps" ofc).

"Maciej Izak" wrote

> Okay, I think we can indeed go with this aside from one small remark:

the double indirection for Pointers is only needed if data resides in different units as those might be in different dynamic packages. Since the INIT and FULL RTTI of a record are always in the same unit a single indirection is sufficient (thus PRecInitTable instead of PPRecInitTable).

IIRC something is (was?) wrong (AV) for Mac i386 and Win64 when is used

PRecInitTable instead of (indirect) PPRecInitTable... It is strange because for example for Win32 and Aarch64 PRecInitTable works fine... Have you an idea?

No idea. But I have a couple of platforms to test this on myself :) (x86_64-win64, i386-win32, powerpc-darwin, i386-linux, x86_64-linux, arm-linux, m68k-linux)

> > Other than that I think we've found a good compromise for our wishes and

hopes for this :)

Phew! So... Please merge PART 2 of MO :P after this step I can start work

on RTTI.pas and on extended info for records (with small "baby steps" ofc).

Okay, will do that either tonight (don't know yet whether I'll have the time however) or hopefully tomorrow evening.

Regards,
Sven

2016-12-13 14:31 GMT+01:00 Sven Barth :

No idea. But I have a couple of platforms to test this on myself :) (x86_64-win64, i386-win32, powerpc-darwin, i386-linux, x86_64-linux, arm-linux, m68k-linux

It is easy to reproduce with PRecInitTable. You can play with "*PART 2* patch" with PRecInitTable/PPRecInitTable pointers and InitializeArray/FinalizeArray for records.

Okay, will do that either tonight (don't know yet whether I'll have the time however) or hopefully tomorrow evening.

^^ I will wait. Not bad info :)

(x86_64-win64, i386-win32, powerpc-darwin, i386-linux, x86_64-linux, arm-linux, m68k-linux

It is easy to reproduce with PRecInitTable. You can play with "*PART 2*

patch" with PRecInitTable/PPRecInitTable pointers and InitializeArray/FinalizeArray for records.

At least the patch as is (with PPointer) didn't trigger anything on Win64...

> > Okay, will do that either tonight (don't know yet whether I'll have the

time however) or hopefully tomorrow evening.

^^ I will wait. Not bad info :)

Committed with a few adjustments. Now I only need to update User Changes Trunk, but I'll do that tomorrow *yawns*

Regards,
Sven

2016-12-14 0:16 GMT+01:00 Sven Barth :

At least the patch as is (with PPointer) didn't trigger anything on Win64... Will it stay as PPointer (PPRecInitTable)? I am wondering why we have

bug (or expected behavior?) for this.

Committed with a few adjustments. Now I only need to update User Changes Trunk, but I'll do that tomorrow *yawns*

Thanks... Now I need to merge "few adjustments" into NewPascal -,-'

Shall we increase CurrentPPUVersion or we will wait some time (we have many changes pending so probably inc for CurrentPPUVersion has not much sense yet)?

"Maciej Izak" wrote

2016-12-14 0:16 GMT+01:00 Sven Barth : > > At least the patch as is (with PPointer) didn't trigger anything on

Win64...

Will it stay as PPointer (PPRecInitTable)? I am wondering why we have

bug (or expected behavior?) for this.

It should be a Pointer of course, not a PPointer... I will correct that later on (I shouldn't commit code that late -.- ).

> > Committed with a few adjustments. Now I only need to update User Changes

Trunk, but I'll do that tomorrow *yawns*

Thanks... Now I need to merge "few adjustments" into NewPascal -,-'

:P

Shall we increase CurrentPPUVersion or we will wait some time (we have

many changes pending so probably inc for CurrentPPUVersion has not much sense yet)?

PPU version is only increased if the structure of the PPU file is changed. That RTTI change has nothing to do with the PPU.

Regards,
Sven

2016-12-14 11:17 GMT+01:00 Sven Barth :

Yes, that is true, but I want to be sure :) , at the end we have change in critical place.

This change is big step forward in right direction.

later on (I shouldn't commit code that late -.- ).

So I will wait again ^^

Oh and should you manage to reproduce the problem you mentioned, please notify me.

> > PPU version is only increased if the structure of the PPU file is

changed. That RTTI change has nothing to do with the PPU.

Yes, that is true, but I want to be sure :) , at the end we have change

in critical place.

The PPU version is only to ensure that the compiler doesn't read the wrong data when reading a PPU due to added contents.

For the development version it's always the user's responsibility to ensure that she doesn't mix compiled units from different trunk revisions.

Regards,
Sven

I found the "problem": write_rtti_ref() will always write an indirect reference (which is by design), so I adjusted the code that it will really write a direct one. Bye, bye, one level of indirection ;)

Regards,
Sven

2016-12-14 20:06 GMT+01:00 Sven Barth :

I found the "problem": write_rtti_ref() will always write an indirect reference (which is by design), so I adjusted the code that it will really write a direct one. Bye, bye, one level of indirection ;)

^_^ but...

I found another problem. We have small typo for
RTTIRecordRttiInfoToInitInfo. Please merge ASAP
http://bugs.freepascal.org/view.php?id=31118 otherwise InitializeArray FinalizeArray will stay totally broken >.<

RTTIRecordRttiInfoToInitInfo. Please merge ASAP
http://bugs.freepascal.org/view.php?id=31118 otherwise InitializeArray FinalizeArray will stay totally broken >.<

Done :)

Regards,
Sven