View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0021636FPCRTLpublic2012-04-03 05:452012-04-25 19:30
ReporterMichalis Kamburelis 
Assigned ToMarco van de Voort 
PrioritynormalSeverityminorReproducibilityalways
StatusresolvedResolutionfixed 
Platformi386OSDebian GNU/LinuxOS Version(testing)
Product VersionProduct Build 
Target VersionFixed in Version2.7.1 
Summary0021636: TFPSList.Pack is completely broken (patch)
Description1. TFPSList.Pack implementation happily assumes that list item size is the same as the Pointer size on current architecture. That's because it checks "assigned(pointer(psrc^))" to see if item should be kept. Needless to say, this is incorrect in many cases (and in some cases, like LongInt, it may be incorrect on 64-bit architectures but accidentally work on 32-bit architectures).

Attached testcase demonstrates how Pack on a list of bytes removes only some of the bytes --- only when it sees 4 following bytes (tested on 32-bit architecture, on 64-bit Pack will probably not do anything).

$ fpc pack_invalid_size.pas
$ ./pack_invalid_size
0: 0
1: 0
2: 0
3: 1
4: 2
5: 0
6: 3
7: 4

2. Also, implementation of TFPSList.Pack does inc(psrc), which means that it moves the psrc by 1 byte. So it doesn't really work even when list item has size = pointer size (4 or 8 bytes usually). Can be tested by replacing Byte with LongInt in pack_invalid_size.pas testcase, and observing that now the result is different but also wrong: list is empty at the end.

3. Also, it seems that it moves items by direct System.Move, without bothering to call Deref or zero memory. So it will probably cause troubles in case of types that require Initialize/Finalize.

I would advice to just remove the current Pack method. It seems broken beyond hope. I also don't see much use for this method. Eventually, maybe a simple and clean (without any pointer magic) TFPGObjectList.Pack can be implemented later as a replacement.
TagsNo tags attached.
FPCOldBugId0
Fixed in Revision 21039
Attached Files? file icon pack_invalid_size.pas [^] (386 bytes) 2012-04-03 05:45
diff file icon fglpack.diff [^] (1,149 bytes) 2012-04-22 13:20 [Show Content]

- Relationships

-  Notes
(0058878)
Marco van de Voort (manager)
2012-04-22 13:21
edited on: 2012-04-22 13:23

I had a look, and the whole concept of fpslist is a bit alien to me, specially the heavy use of pointers, so I won't commit it without some testing and/or other eyes.

The fact that compare is not based on value comparison, makes some optimizations (for small types) impossible, which makes this class uninteresting.

Anyway, with the patch, the output is (I assume) correct:

0: 1
1: 2
2: 3
3: 4

(0058903)
Michalis Kamburelis (reporter)
2012-04-22 21:06

The patch looks good IMHO. It fixes 1st and 2nd problems. The 3rd problem mentioned in my report is probably not an issue --- as things filled with zero memory do not need to be freed/finalized anyway. As long as someone doesn't override and use Deref for other tricks, it all should be Ok.

And I totally agree about the ugliness of TFPSList based on pointers. It would be much cleaner to just implement all operations straight in TFPGList, without pointer magic, without intermediate TFPSList class. But then, TFPGObjectList would either need to have a separate implementation from TFPGList (long code duplicated) or TFPGObjectList would need to descend from TFPGList (which is not possible for now --- one generic cannot descend, and extend, from other generic; this was determined as wontfix e.g. in 0008591 , 0009007). I dream about cleaner FGL implementation too, and I would probably choose to duplicate code for TFPGObjectList for now, instead of keeping unclean TFPSList.
(0058906)
Marco van de Voort (manager)
2012-04-22 22:59

I don't see why it doesn't fix issue 3. It now derefs killed images, and this routine does not create new instances, so no initialization of memory is needed.

(the move overrides regardless of the target being zeroed or not)
(0058909)
Michalis Kamburelis (reporter)
2012-04-22 23:31

Ah yes, when reading the patch I just didn't notice the new "deref(psrc)" calls, sorry. Excellent, so it fixes everything :)
(0058976)
Marco van de Voort (manager)
2012-04-25 19:25
edited on: 2012-04-25 19:26

Committed, since in any case it is better then what is there. For the rest, we'll see if generics are fully up to snuff.

Delphi afaik supports inheritance of generic classes.

(0058978)
Marco van de Voort (manager)
2012-04-25 19:30

I reopened 9007, since the situation changed because of Delphi generics, and the people currently working on generics might not even have seen it.

- Issue History
Date Modified Username Field Change
2012-04-03 05:45 Michalis Kamburelis New Issue
2012-04-03 05:45 Michalis Kamburelis File Added: pack_invalid_size.pas
2012-04-22 13:20 Marco van de Voort File Added: fglpack.diff
2012-04-22 13:20 Marco van de Voort FPCOldBugId => 0
2012-04-22 13:20 Marco van de Voort Summary TFPSList.Pack is completely broken => TFPSList.Pack is completely broken (patch)
2012-04-22 13:21 Marco van de Voort Note Added: 0058878
2012-04-22 13:22 Marco van de Voort Note Edited: 0058878
2012-04-22 13:22 Marco van de Voort Note Edited: 0058878
2012-04-22 13:23 Marco van de Voort Note Edited: 0058878
2012-04-22 13:24 Marco van de Voort Status new => confirmed
2012-04-22 21:06 Michalis Kamburelis Note Added: 0058903
2012-04-22 22:59 Marco van de Voort Note Added: 0058906
2012-04-22 23:31 Michalis Kamburelis Note Added: 0058909
2012-04-25 19:25 Marco van de Voort Fixed in Revision => 21039
2012-04-25 19:25 Marco van de Voort Status confirmed => resolved
2012-04-25 19:25 Marco van de Voort Fixed in Version => 2.7.1
2012-04-25 19:25 Marco van de Voort Resolution open => fixed
2012-04-25 19:25 Marco van de Voort Assigned To => Marco van de Voort
2012-04-25 19:25 Marco van de Voort Note Added: 0058976
2012-04-25 19:26 Marco van de Voort Note Edited: 0058976
2012-04-25 19:30 Marco van de Voort Note Added: 0058978



MantisBT 1.2.12[^]
Copyright © 2000 - 2012 MantisBT Group
Powered by Mantis Bugtracker