Possible issue in Fonts.pm with perl 5.24

[stefansielaff]

Hi, I would like to let you know that there might be an issue (with perl 5.24) in Fonts.pm. I'm not deep enough in perl but the ternary expression selecting old BidiL/R seems to cause a fail at compile time whenever a box with display connects.

Here's an excerpt of a log file: (https://ptpb.pw/MQmG) As I say I dont want to tinker with your perl code, as support for 5.8 isn't neccesary in my case I've removed the expression from my fork and the problems are gone (https://github.com/stefansielaff/slimserver/commit/1011df8c971aa7171cb065e8d1cd1fc6193fc1f0).

[mherger]

Thanks for the heads up. Though this is pretty odd: the version check fails - which shouldn't. What would the following line give you?

perl -e 'print $]'

[stefansielaff]

That was my first guess too, but it shows 5.024000 as expected.

Even this: my $test = ($] <= 5.008004) ? "OLD" : "NEW"; logWarning($test); outputs "NEW".

It looks like 5.24 either changed the way it evaluates such expressions and now checks them at compile time or maybe they finally dropped the old constants?

[mherger]

Thanks for this test. That's really odd. I'll have to look into this. Maybe it's actually a change in an earlier Perl version already, but only got exposed now by this change (from the Perl 5.24 changelog):

Regular expression compilation errors

Some regular expression patterns that had runtime errors now don't compile at all.

Almost all Unicode properties using the \p{} and \P{} regular expression pattern constructs are now checked for validity at pattern compilation time, and invalid ones will cause the program to not compile. In earlier releases, this check was often deferred until run time. Whenever an error check is moved from run- to compile time, erroneous code is caught 100% of the time, whereas before it would only get caught if and when the offending portion actually gets executed, which for unreachable code might be never.

[stefansielaff]

Mhhh, as a (Java...) developer I wouldn't expect such a kind of fundamental change in a minor update of the compiler. The "good" news is most of the Arch Linux users are on the move to 5.24 and this was the only problem reported so far. Thanks for looking into this, wish I could help more than testing but it's... perl you know ;)

[udif]

@mherger , This is very interesting. If I understand what you write correctly, the code fails to compile under 5.24 not because it was Illegal under 5.8.4 but because it is illegal on 5.24 even though it was legal on 5.8.4 (I just spend 15 minutes finding 5.8.4 and pulling the docs). from what I can tell the original code that was supposed to be used for 5.8.4 (or earlier) is correct for that version. Is there a way to defer this to runtime, where it will be legal on 5.8.4?

I'm interested by that because several years ago I noticed that BiDi support was broken my NAS units, while it was working perfectly on my Windows PC. At the time I tried solving this but gave up, living with reverse text.

UPDATE: I just checked both of my NAS units - one runs 5.8.8 (Ancient Buffalo LS250GL with upgraded HD) and one runs 5.10.1 (WD MyBookLive), so as much as I hope, perhaps this is not the issue in my case, and so I probably don't need to worry about this 5.8.4 support, as I won't need it after all.

[mherger]

This is very interesting. If I understand what you write correctly, the code fails to compile under 5.24 not because it was Illegal under 5.8.4 but because it is illegal on 5.24 even though it was legal on 5.8.4

Not exactly. What I'm baffled about is the fact that 5.24 seems to be using what should be used with 5.8.4 and older, rather than what's supposed to be used with newer versions. As if it failed the version check.

[stefansielaff]

I think this is not quite correct though. It would select the correct expression but it doesn't get this far anymore.

My english might be insufficient, but I'll try to explain what I mean anyway... The perldelta says they moved the sysntax check of the expressions from runtime to compile time.

<5.24: The regex hasn't been tested by the compiler until the script was executed and evaluated the ternary expression. According to the result of the ternary it selected the old or the new regex and compiled only the selected one at that moment (which was always the right one because the selection works)

5.24 and newer: The compiler now evaluates all regular expressions at the moment of compilation. So every regex used must be valid for the compiler before the script even starts to run and regardless of being used or not. Every regex is now treated as if you would write code and treat invalid ones like a typo in "prent" instead of "print". So the compiler starts and sees the "now invalid" regex and says: STOP, there's some invalid code, i won't even start thinking about the ternary expression before.

For what I've read there's the possibility to defer code compilation to runtime using the "eval" function?

[mherger]

@stefansielaff - Oh... only now that I wanted to type a response how that was wrong did I understand: the regex is compiled before the condition is hit. That's interesting, as I thought that Perl was smart enough to ignore blocks which were not reachable based on some constant value (eg. all those "if (main::SCANNER)" etc. statements). But then I must assume that the ternary expression behaves differently. Could you please try the following?

 # Bug 3535 - perl < 5.8.5 uses a different Bidi property class.
-my $bidiR = ($] <= 5.008004) ? qr/\p{BidiR}/ : qr/\p{BidiClass:R}/;
-my $bidiL = ($] <= 5.008004) ? qr/\p{BidiL}/ : qr/\p{BidiClass:L}/;
+my $bidiR;
+my $bidiL;
+if ($] <= 5.008004) {
+   $bidiR = qr/\p{BidiR}/;
+   $bidiL = qr/\p{BidiL}/;
+}
+else {
+   $bidiR = qr/\p{BidiClass:R}/;
+   $bidiL = qr/\p{BidiClass:L}/;
+}

[stefansielaff]

Unfortunately the same result, it's not the ternary expression, it's defining an "invalid" regex in the code:

Can't find Unicode property definition "BidiR" in regex; marked by <-- HERE in m/\p{BidiR} <-- HERE / at /opt/logitechmediaserver/Slim/Display/Lib/Fonts.pm line 110.

Here is a fix I'd suggest, it defers the selection of a string from compiling the regex as a string:

my $sBidiR = ($] <= 5.008004) ? '\p{BidiR}' : '\p{BidiClass:R}';
my $bidiR = qr/$sBidiR/;
my $sBidiL = ($] <= 5.008004) ? '\p{BidiL}' : '\p{BidiClass:L}';
my $bidiL = qr/$sBidiL/;

[stefansielaff]

Honestly I don't know how to test this, I've tried:

my $test = "Lorem Ipsum";
logWarning("R:". ($test =~ $bidiR));
logWarning("L:". ($test =~ $bidiL));

my $test = "והוא ערבית ופיתוחה על";
logWarning("R:". ($test =~ $bidiR));
logWarning("L:". ($test =~ $bidiL));

my $test = "فكانت مشاركة بـ أخذ, بين حل ّت الأثناء، و";
logWarning("R:". ($test =~ $bidiR));
logWarning("L:". ($test =~ $bidiL));

and the output is:

[16-06-15 13:15:08.4755] Slim::Display::Graphics::BEGIN (108) Warning: R:
[16-06-15 13:15:08.4757] Slim::Display::Graphics::BEGIN (109) Warning: L:1
[16-06-15 13:15:08.4759] Slim::Display::Graphics::BEGIN (112) Warning: R:
[16-06-15 13:15:08.4761] Slim::Display::Graphics::BEGIN (113) Warning: L:1
[16-06-15 13:15:08.4763] Slim::Display::Graphics::BEGIN (116) Warning: R:
[16-06-15 13:15:08.4765] Slim::Display::Graphics::BEGIN (117) Warning: L:1

No difference, but I'm not sure if my copy & paste rtl language should be recognized as is. Maybe it's a problem on my linux testbox like @udif mentioned above.

[otherjon]

@stefansielaff , @mherger -- Thank you for your work on this, since I'm getting the same error and would love to see it fixed. @stefansielaff , I can get a simplified test to pass if I include use utf8; at the top of my test script. (That use utf8; would probably need to be added to the top of Fonts.pm, in addition to your patch.)

% perl -e 'print "Version: $]\n";'
Version: 5.024000
% perl -e 'print ( "Test: ", ("יתוחהעל" =~ /\p{BidiClass:R}/) ? "match" : "no match", "\n");'
Test: no match
% perl -e 'use utf8; print ( "Test: ", ("יתוחהעל" =~ /\p{BidiClass:R}/) ? "match" : "no match", "\n");'
Test: match

Not entirely sure what's going on, because the doc seems to imply that use utf8; shouldn't be necessary here -- as described in http://perldoc.perl.org/perluniintro.html under "Perl's Unicode Support". But it seems like a valid workaround.

[mherger]

@otherjon - great hint. I did indeed still find the "use utf8" in some places in our code, together with a comment that this was important due to some regexes.... So, could you please try the following change?

diff --git a/Slim/Display/Lib/Fonts.pm b/Slim/Display/Lib/Fonts.pm
index 8611e88..b6de93a 100644
--- a/Slim/Display/Lib/Fonts.pm
+++ b/Slim/Display/Lib/Fonts.pm
@@ -98,6 +98,9 @@ tie my %TTFCache, 'Tie::Cache::LRU', 256;
 # template for unpacking strings: U - unpacks Unicode chars into ords, C - is needed for 5.6 perl's
 my $unpackTemplate = ($] > 5.007) ? 'U*' : 'C*';

+# important for regexps that follow
+use utf8;
+
 # Bug 3535 - perl < 5.8.5 uses a different Bidi property class.
 my $bidiR = ($] <= 5.008004) ? qr/\p{BidiR}/ : qr/\p{BidiClass:R}/;
 my $bidiL = ($] <= 5.008004) ? qr/\p{BidiL}/ : qr/\p{BidiClass:L}/;

[mherger]

Commit https://github.com/Logitech/slimserver/commit/ecc309936d7d40fb8170ef89f1e0b94fe11f15be.

I've removed that check. Perl 5.8.6 has been out for 12 years. We shouldn't need to worry about anyone using an older build.