| Anonymous | Login | Signup for a new account | 2013-05-21 11:44 CEST | ![]() |
| All Projects | FPC | Lazarus: Packages, Patches | Lazarus CCR | Mantis | fpGUI | fpcprojects: fpprofiler |
| Main | My View | View Issues | Change Log | Roadmap |
| View Issue Details [ Jump to Notes ] | [ Issue History ] [ Print ] | ||||||||
| ID | Project | Category | View Status | Date Submitted | Last Update | ||||
| 0021636 | FPC | RTL | public | 2012-04-03 05:45 | 2012-04-25 19:30 | ||||
| Reporter | Michalis Kamburelis | ||||||||
| Assigned To | Marco van de Voort | ||||||||
| Priority | normal | Severity | minor | Reproducibility | always | ||||
| Status | resolved | Resolution | fixed | ||||||
| Platform | i386 | OS | Debian GNU/Linux | OS Version | (testing) | ||||
| Product Version | Product Build | ||||||||
| Target Version | Fixed in Version | 2.7.1 | |||||||
| Summary | 0021636: TFPSList.Pack is completely broken (patch) | ||||||||
| Description | 1. 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. | ||||||||
| Tags | No tags attached. | ||||||||
| FPCOldBugId | 0 | ||||||||
| Fixed in Revision | 21039 | ||||||||
| Attached Files | |||||||||
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 | |
| Main | My View | View Issues | Change Log | Roadmap |



