kryslt / KControls

Free components for Delphi and Lazarus, this is the main repository maintained by the original author.
BSD 3-Clause Clear License
59 stars 32 forks source link

Not FPC-compatible uses of case..of #33

Closed martok closed 3 years ago

martok commented 3 years ago

Code segments like this trigger a language difference between Delphi and Freepascal: https://github.com/kryslt/KControls/blob/c1cb2767a21b96a415b114ee4697e8038034de69/source/kmemortf.pas#L2484

Freepascal has some opinions about what values a case on an enumerated type handles. In this case, this causes an invalid jump table jump to be generated that is hit for example by this call: https://github.com/kryslt/KControls/blob/c1cb2767a21b96a415b114ee4697e8038034de69/source/kmemortf.pas#L2733 In a few instances, the same "logic" causes FPC to raise warning "unreachable code" in else-branches of case statements where it thinks that all possible values had been listed.

Now, it is clear that FPC is obviously wrong here and violates just about every Pascal language standard, but since Core has decided that this won't be fixed and keeps repeating that on every related bug report about once a year, one has to insert these checks in code. So basically, every single one of these cast-cases needs a range check on aCtrl...

kryslt commented 3 years ago

This is a FPC problem, package normally compilable. Closing this issue.

kryslt commented 3 years ago

I'm reopening this problem because I've studied it in detail and tried to debug and find out what's going on there. Finally, I was able to open the RTF file that caused the invalid jump. Now I'm not sure if this is a problem with FPC or not. More and more it seems to me that it's really a bug in my code. Either way, I will solve this by adding another empty enumerator to avoid these invalid jumps.

martok commented 3 years ago

Well, one might argue that in this specific case (you use "dense" enums instead of "C-style" enums) the compiler has some room for interpretation. But it's certainly different from Delphi. Unchecked jumps into jumptables smaller than a subregister are just flat out unsafe, no compiler other than FPC generates them.

Either way, I will solve this by adding another empty enumerator to avoid these invalid jumps.

~How do you mean? The first few intuitive alternatives are usually also removed by the compiler...~ Ah, right. That should be safe for now.

I have been running a patched compiler for this for years and only noticed the problem because the patch wasn't active in a build...

kryslt commented 3 years ago

I did not dig into compilation and assembly details much and did not compare with Delphi compiler, either. I've found with optimization -O2 that for some case statements the FPC compiler creates jump tables (completely unchecked as you say) and for some if-else-if blocks (if this has some scientific name please correct me) where these are always checked by design.

For more case labels jump tables were created, so there must be some threshold given by the optimizer.

With if-else-if blocks no problem (eg. ReadFontGroup(-1,...)), with jump tables there was problem (eg. ReadPictureGroup(-1,...).

So finally we can argue the bug is present in FPC and the bug is (was) also present in my code. From my part I hopefully fixed this.

These two methods of compiling case statements (jump tables and if-else-if blocks) should be completely transparent to the programmer and their behavior should be the same, regardless of optimization level.

martok commented 3 years ago

These two methods of compiling case statements (jump tables and if-else-if blocks) should be completely transparent to the programmer and their behavior should be the same, regardless of optimization level.

In an ideal world, that would be the case.

At O1 or above, there are thresholds between jumplists (O(n)), jumptrees (O(log n)) and jumptables (O(1)). The first two behave identical, the last one doesn't.

But yeah, making sure that there is naver a value that has no name in the enum fixes the problem and should be stable even if the compiler changes at some point. Thanks!