View Issue Details

IDProjectCategoryView StatusLast Update
0013700FPCCompilerpublic2013-06-02 12:45
ReporterMichael Vadymovitch Denisenko Assigned ToFlorian  
PrioritynormalSeverityfeatureReproducibilityhave not tried
Status closedResolutionfixed 
Product Version2.2.2 
Fixed in Version2.6.0 
Summary0013700: Case of string
Description1st patch. Some functions used in parse case of string
2nd patch. Parse case of string
3rd patch. Some tests for parser
4th patch. Code generation for case of string
5th patch. Tests for code generator
TagsNo tags attached.
Fixed in Revision14467
FPCOldBugId
FPCTarget
Attached Files

Relationships

parent of 0013620 closedJonas Maebe Four patches for 'case of string'. 
parent of 0014734 closedFlorian Case of string: bugfix. 
related to 0014070 closedJonas Maebe Make case understand TClass descedants 

Activities

2009-05-13 12:51

 

Case_of_string.zip (19,999 bytes)

Florian

2009-05-13 22:34

administrator   ~0027575

The on-the-fly generation of nodes inside the code generator is *very bad* coding style. Why don't you simply convert the whole string case statement inside the firstpass into if nodes and let the if node code do the rest?

Jonas Maebe

2009-05-17 22:35

manager   ~0027673

Creating new nodes in the code generator pass is indeed not done.

Something else, possibly for later on: this seems like a textbook example where using a "perfect hash" would be a great time saver (at run time). See, e.g., http://en.wikipedia.org/wiki/Perfect_hash_function

Michael Vadymovitch Denisenko

2009-05-18 07:09

reporter   ~0027687

Maybe it's better to implement PATRICIA? Imho perfect hash is too complex to be implemented on a rather small set of case values.

Jonas Maebe

2009-05-18 22:12

manager   ~0027718

That's indeed also possible. But for a first implementation, simply using a bunch of if-then-else nodes is fine.

2009-05-19 08:41

 

Case_of_string_2.zip (19,316 bytes)

Michael Vadymovitch Denisenko

2009-05-19 08:42

reporter   ~0027739

New patch series is added.

Florian

2009-06-06 23:28

administrator   ~0028343

Why do you create the initblock in the parser? Just create it in the code generator pass 1 on the fly if needed.

2009-06-08 07:11

 

Case_of_string_3.zip (3,159 bytes)

Michael Vadymovitch Denisenko

2009-06-08 07:12

reporter   ~0028361

Only patches with parser and code generator added, others not changed.

Florian

2009-06-08 11:07

administrator   ~0028364

Looks better ;) However, I think (or did I miss it :)?) ppuwritecaselabel and ppuloadcaselabel are not extended to support case lables with strings yet? You can test this by putting a string case statement into an inline subroutine in a unit and use this inline subroutine in another program.

2009-08-12 17:07

 

Case_of_string_4.zip (1,026 bytes)

Michael Vadymovitch Denisenko

2009-08-12 17:09

reporter   ~0029786

Sorry for delay, now all seems to be good.

Florian

2009-08-12 18:33

administrator   ~0029792

n*.pas units should not depend on fmodule, is there a reason why the label string cannot be deleted? Do you have any tests?

Michael Vadymovitch Denisenko

2009-08-13 08:30

reporter   ~0029810

As I understood, the procedure which deletes labels (deletecaselabels) is called once if we compile program and twice if we have a unit. The ppuwrite for labels is called between first and second calls of deletecaselabels, so we must skip the first call to keep the label value safe until it will be written into module and delete it only on the second call.

The tests will be included into the next archive.

Michael Vadymovitch Denisenko

2009-08-14 06:05

reporter   ~0029837

Last edited: 2009-08-14 14:13

Attached new patches: tests and modifications in ppuwrite/ppuload providing ability to use wide/unicode strings forgotten by me at first time.

However, use of wide/unicode strings fails with internal error 200208151 on tests included in archive even when patches are not applied at all.

fmodule is still used; is it available to make check for delete without it?

2009-08-14 06:05

 

Case_of_string_5.zip (2,357 bytes)

Florian

2009-08-16 10:09

administrator   ~0029901

Well, we should find out why deletecaselabels is called twice.

Michael Vadymovitch Denisenko

2009-08-18 05:21

reporter   ~0029948

Last edited: 2009-08-18 05:22

The 'double-deletion' is caused by copying of _inline_ function code.

When we parse module the proc_unit procedure is called. Then it calls parse_body, which reads declarations and procedure bodies (read_proc_body).

From the last procedure the call of parse_body takes place, and in parse_body the block which's the body of our inline function is _copied_ to the procdef. But when we return to the read_proc_body here this block is immediately generated, what causes call of pass_1.

As we are changing case_of_string into if_then_else the case_of_string is then deleted in pass_1; that's the first call of deletecaselabels. However, the copied block remains the same.

When we return to proc_unit and try to write data into ppu it fails: labels are already freed. But even if we skip this error the compilation will fail on DoneParser when loaded_units free takes place - that's the second call of deletecaselabels.

Michael Vadymovitch Denisenko

2009-08-18 05:53

reporter   ~0029949

Oh, I've got it. I've forgotten about copycaselabels; now it's ok.

2009-08-18 05:54

 

Case_of_string_6.zip (1,665 bytes)

2009-08-20 08:44

 

Case_of_string_7.zip (1,657 bytes)

Michael Vadymovitch Denisenko

2009-08-20 08:45

reporter   ~0030009

Deleted use of unit fmodule; updated patch is in archive Case_of_string_7.zip

Florian

2009-08-20 11:55

administrator   ~0030014

I'll look into the integration.

Florian

2009-08-20 23:52

administrator   ~0030043

The copying of case labels does not take care of widestring/unicode strings yet! Further, the allocated memory is too short (strlen instead of strlen+1)

2009-08-22 13:31

 

Case_of_string_8.zip (4,902 bytes)

Michael Vadymovitch Denisenko

2009-08-22 13:33

reporter   ~0030074

Rewrote new tests, reallocated memory and added copying of labels of wide-/uni-.

Florian

2009-09-03 22:23

administrator   ~0030384

I've committed the patch in r13642, thanks. If you feel bored the following improvements could be done:
- fix ppudump to understand the new case nodes
- chain similiar tests which must be successfully executed into one test, this reduces regression testing time which is around 10 min currently making development often boring

Jonas Maebe

2009-09-04 10:10

manager   ~0030393

I think there may also be a memory management error with get_string_value: usually it allocates a new pchar, but sometimes it returns an existing one from somewhere else (e.g., "get_string_value := tstringconstnode(p).value_str;") or a constant one (in case of error, a constant empty string is returned).

In all cases, the result is used to create a new case label, and the case label frees the strings when it is destroyed. This can result in invalid or double frees.

The code indentation in the change to pstatment.pas is also wrong: http://svn.freepascal.org/cgi-bin/viewvc.cgi/trunk/compiler/pstatmnt.pas?r1=13642&r2=13641&pathrev=13642 (see near the bottom)

2009-09-07 02:03

 

Case_of_string_9.zip (2,774 bytes)

Michael Vadymovitch Denisenko

2009-09-07 02:04

reporter   ~0030497

Now get_string_value returns only allocated pchars. pstatmnt.pas corrected.

Florian

2009-09-07 08:50

administrator   ~0030501

Could you rebase the patch against the current svn head?

2009-09-07 14:02

 

Michael Vadymovitch Denisenko

2009-09-07 14:04

reporter   ~0030511

As I understood the patch was to contain only new changes; the last archive contains them.

Florian

2009-09-14 23:44

administrator   ~0030719

Applied Case_of_string_final.zip to svn in r13712.

Jonas Maebe

2009-09-14 23:56

manager   ~0030720

Sorry for only reviewing the patch now that it's applied, but it's wrong:
a) you cannot use strcopy on the value_str field of stringconstnode, because those strings can contain #0's in the middle, and moreover they are not guaranteed to be null-terminated. You have to use move() instead
b) the "in case of error, a constant empty string is returned" I mentioned before is still the case.

2009-09-15 11:57

 

Michael Vadymovitch Denisenko

2009-09-15 11:58

reporter   ~0030733

All problems seem to be fixed.

Jonas Maebe

2009-09-15 12:07

manager   ~0030735

The parameters of the move are in the wrong order: it's move(source,dest,len), not move(dest,source,len). Other than that it looks correct.

2009-09-15 12:13

 

Michael Vadymovitch Denisenko

2009-09-15 12:21

reporter   ~0030736

Sorry for my inattention. Fix is uploaded.

Florian

2009-09-15 23:06

administrator   ~0030759

Applied the last fix in r13718

Jonas Maebe

2009-09-16 08:43

manager   ~0030767

It seems to have broken most string case tests: http://www.freepascal.org/testsuite/cgi-bin/testsuite.cgi?action=1&run1id=43210&run2id=43251&noskipped=1

Michael Vadymovitch Denisenko

2009-09-16 16:06

reporter   ~0030772

That's strange. I've just very carefully tested all case tests (tcase1.pp - tcase48_2.pp) and found nothing wrong.

However, I've uploaded the log generated by the testing system written especially for this case; maybe it could be useful.
The one reason in the difference of the operating systems comes to mind (tests we run under Win32).

2009-09-16 16:07

 

run.log (26,797 bytes)

Florian

2009-09-16 21:59

administrator   ~0030781

Fixed.

Jonas Maebe

2009-09-16 22:02

manager   ~0030782

I guess this bug report can now be closed, no?

Florian

2009-09-16 22:09

administrator   ~0030783

ppudump still needs to be fixed.

Sergei Gorelkin

2009-09-20 10:39

developer   ~0030855

I want to complain about poor quality of this code:

1) Building the compiler with OPT=-gh and then compiling any test case containing case-of-string shows a number of leaks and at least one invalid memory access.

2) Using StrComp to compare ansi- and shortstring expressions carries the same problem as allocating them with StrCopy - it will stop on embedded zero.

3) The test cases 15..17, 19..27, 31..33, 35..44 have variables named my_str, my_str_wide, my_str_ansi and my_str_uni all declared as 'string', while their names suggests that they should be of different types (and this is indeed so in other tests, e.g. tcase18). This can potentially lead to some errors going undetected.

Florian

2009-10-09 22:41

administrator   ~0031238

Sergei, could be please check if r13830 resolves your concerns?

2009-10-10 15:47

 

tcasez.pp (333 bytes)

Sergei Gorelkin

2009-10-10 16:14

developer   ~0031247

I'm afraid that no:

1) Memory leaks are gone for positive tests, but remain in place for the tests that should fail (tcase19.pp being one example).

2) Not solved. I attached a test example (tcasez.pp) that fails; the first part of test demonstrates that using regular comparisons instead of case works.

3) Solved.


Regarding the string comparison: the newly written routine compare_strings is actually a clone of strcomp. Using Length(PChar) has the same effect as using StrLen.
The reason of choosing PChar for storing the case labels, stripping the length which is stored in the tstringconstnode, then trying to recover, converting to TConstString and ending up again in tstringconstnode is unclear to me.
IMHO the stringconstnodes (or, their copies created by getcopy()) created by parser should be attached to tcaselabel's and then used in the generated tree. The type of tstringconstnode can be changed from ansi to wide by its changestringtype() method. Then, the only remaining thing is conversion from single chars to strings.

Michael Vadymovitch Denisenko

2009-10-11 06:03

reporter   ~0031257

Firstly I've chosen pchar to avoid storing of useless information. But now I see it's actually required.

2009-12-18 13:03

 

Sergei Gorelkin

2009-12-18 13:13

developer   ~0033170

Last edited: 2009-12-18 13:15

Since no progress within two months, I dare fix it up myself.

Attached is a patch (13700_cleanup_patch.zip) that drops all the junk and makes comparisons of strings with zeroes work correctly. Also fixes memory leaks on negative tests for both ordinal and string casenodes (actually, 'normal' case statements with duplicate labels were also leaking, there were simply no tests to trigger that).

tcase0.pp is a basic test for strings with embedded zeroes, in format suitable for the testsuite.

2009-12-18 13:13

 

tcase0.pp (629 bytes)

Florian

2009-12-23 20:28

administrator   ~0033287

Thanks, applied.

Issue History

Date Modified Username Field Change
2009-05-13 12:51 Michael Vadymovitch Denisenko New Issue
2009-05-13 12:51 Michael Vadymovitch Denisenko File Added: Case_of_string.zip
2009-05-13 22:34 Florian Note Added: 0027575
2009-05-17 22:35 Jonas Maebe Note Added: 0027673
2009-05-17 22:35 Jonas Maebe Status new => feedback
2009-05-17 22:37 Jonas Maebe Relationship added parent of 0013620
2009-05-18 07:09 Michael Vadymovitch Denisenko Note Added: 0027687
2009-05-18 22:12 Jonas Maebe Note Added: 0027718
2009-05-19 08:41 Michael Vadymovitch Denisenko File Added: Case_of_string_2.zip
2009-05-19 08:42 Michael Vadymovitch Denisenko Note Added: 0027739
2009-06-06 23:28 Florian Note Added: 0028343
2009-06-08 07:11 Michael Vadymovitch Denisenko File Added: Case_of_string_3.zip
2009-06-08 07:12 Michael Vadymovitch Denisenko Note Added: 0028361
2009-06-08 11:07 Florian Note Added: 0028364
2009-08-12 17:07 Michael Vadymovitch Denisenko File Added: Case_of_string_4.zip
2009-08-12 17:09 Michael Vadymovitch Denisenko Note Added: 0029786
2009-08-12 18:33 Florian Note Added: 0029792
2009-08-13 08:30 Michael Vadymovitch Denisenko Note Added: 0029810
2009-08-14 06:05 Michael Vadymovitch Denisenko Note Added: 0029837
2009-08-14 06:05 Michael Vadymovitch Denisenko File Added: Case_of_string_5.zip
2009-08-14 14:13 Michael Vadymovitch Denisenko Note Edited: 0029837
2009-08-16 10:09 Florian Note Added: 0029901
2009-08-17 16:04 Florian Relationship added related to 0014070
2009-08-18 05:21 Michael Vadymovitch Denisenko Note Added: 0029948
2009-08-18 05:22 Michael Vadymovitch Denisenko Note Edited: 0029948
2009-08-18 05:53 Michael Vadymovitch Denisenko Note Added: 0029949
2009-08-18 05:54 Michael Vadymovitch Denisenko File Added: Case_of_string_6.zip
2009-08-20 08:44 Michael Vadymovitch Denisenko File Added: Case_of_string_7.zip
2009-08-20 08:45 Michael Vadymovitch Denisenko Note Added: 0030009
2009-08-20 11:55 Florian Note Added: 0030014
2009-08-20 23:52 Florian Note Added: 0030043
2009-08-22 13:31 Michael Vadymovitch Denisenko File Added: Case_of_string_8.zip
2009-08-22 13:33 Michael Vadymovitch Denisenko Note Added: 0030074
2009-09-03 22:23 Florian Note Added: 0030384
2009-09-04 10:10 Jonas Maebe Note Added: 0030393
2009-09-07 02:03 Michael Vadymovitch Denisenko File Added: Case_of_string_9.zip
2009-09-07 02:04 Michael Vadymovitch Denisenko Note Added: 0030497
2009-09-07 08:50 Florian Note Added: 0030501
2009-09-07 14:02 Michael Vadymovitch Denisenko File Added: Case_of_string_final.zip
2009-09-07 14:04 Michael Vadymovitch Denisenko Note Added: 0030511
2009-09-14 23:44 Florian Note Added: 0030719
2009-09-14 23:56 Jonas Maebe Note Added: 0030720
2009-09-15 11:57 Michael Vadymovitch Denisenko File Added: Case_of_string_final_2.zip
2009-09-15 11:58 Michael Vadymovitch Denisenko Note Added: 0030733
2009-09-15 12:07 Jonas Maebe Note Added: 0030735
2009-09-15 12:13 Michael Vadymovitch Denisenko File Added: Case_of_string_final_3.zip
2009-09-15 12:21 Michael Vadymovitch Denisenko Note Added: 0030736
2009-09-15 23:06 Florian Note Added: 0030759
2009-09-16 08:43 Jonas Maebe Note Added: 0030767
2009-09-16 16:06 Michael Vadymovitch Denisenko Note Added: 0030772
2009-09-16 16:07 Michael Vadymovitch Denisenko File Added: run.log
2009-09-16 21:59 Florian Note Added: 0030781
2009-09-16 22:02 Jonas Maebe Note Added: 0030782
2009-09-16 22:09 Florian Note Added: 0030783
2009-09-20 10:39 Sergei Gorelkin Note Added: 0030855
2009-10-09 13:38 Jonas Maebe Relationship added parent of 0014734
2009-10-09 22:41 Florian Note Added: 0031238
2009-10-10 15:47 Sergei Gorelkin File Added: tcasez.pp
2009-10-10 16:14 Sergei Gorelkin Note Added: 0031247
2009-10-11 06:03 Michael Vadymovitch Denisenko Note Added: 0031257
2009-12-18 13:03 Sergei Gorelkin File Added: 13700_cleanup_patch.zip
2009-12-18 13:04 Sergei Gorelkin File Added: tcase0.pp
2009-12-18 13:13 Sergei Gorelkin Note Added: 0033170
2009-12-18 13:13 Sergei Gorelkin File Deleted: tcase0.pp
2009-12-18 13:13 Sergei Gorelkin File Added: tcase0.pp
2009-12-18 13:15 Sergei Gorelkin Note Edited: 0033170
2009-12-23 20:28 Florian Fixed in Revision => 14467
2009-12-23 20:28 Florian Status feedback => resolved
2009-12-23 20:28 Florian Fixed in Version => 2.5.1
2009-12-23 20:28 Florian Resolution open => fixed
2009-12-23 20:28 Florian Assigned To => Florian
2009-12-23 20:28 Florian Note Added: 0033287
2010-11-13 20:35 Jonas Maebe Status resolved => closed