Broken on ARM (char is unsigned).

[bungeman]

With commit 9b05db3 "[ragel] Regenerate ragel-generated files using ragel 7.0.0.11 May 2018" compiling for ARM (or other platforms where char is unsigned) produces the (rather helpful warning as error here)

FAILED: obj/third_party/harfbuzz-ng/harfbuzz_source/hb-ot-shape-complex-myanmar.o 
In file included from ../../third_party/harfbuzz-ng/src/src/hb-ot-shape-complex-myanmar.cc:136:
NONE:13:57: error: result of comparison of constant -1 with expression of type 'const char' is always true [-Werror,-Wtautological-constant-out-of-range-compare]
                                        if ( _myanmar_syllable_machine_eof_cond_spaces[cs] != -1 ) {
                                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~
1 error generated.

FAILED: obj/third_party/harfbuzz-ng/harfbuzz_source/hb-ot-shape-complex-indic.o 
In file included from ../../third_party/harfbuzz-ng/src/src/hb-ot-shape-complex-indic.cc:377:
NONE:13:55: error: result of comparison of constant -1 with expression of type 'const char' is always true [-Werror,-Wtautological-constant-out-of-range-compare]
                                        if ( _indic_syllable_machine_eof_cond_spaces[cs] != -1 ) {

Note that there is no complaint when these arrays are filled with '-1' since that's just a 'normal' conversion, but on ARM those '-1' in the initialization will actually store '255'. In this comparison both sides will be promoted to 'int', which means this will be actually comparing '255' and '-1' on ARM, leading to different behavior.

[bungeman]

It appears this may have been discussed upstream at http://www.colm.net/pipermail/ragel-users/2019-January/003588.html .

[bungeman]

I sent a patch to the ragel mailing list asking for it to emit 'signed char' when a signed one is what is really desired (where it is obviously desired in codegen.cc). However, when I built harfbuzz with a tip of tree colm/ragel I didn't end up with the '-1' issue in the generated header. It appears that perhaps other changes may have worked around this particular issue.

[behdad]

Thanks Ben. I noticed this least week and communicated with Ragel author, who verified that it's fixed on master.

I will revert the ragel 7 upgrade. It's a pain that Fedora decided to skip that version when it's clearly marked unstable.

Alternatively he suggested we can switch to other codegen modes. Maybe that's easier. I'll try that soon.

[behdad]

For the first time I'm actually glad we're keeping generated files in tree, so we notice changes.

[bungeman]

Alas, this removed the issue from some of the files, but it still exists in others. See https://chromium-review.googlesource.com/c/chromium/src/+/1608701 for the failures after the above change.

[behdad]

Which URL do I check? I checked one of the failures and that's this:

../../third_party/harfbuzz-ng/src/src/hb-ot-shape-complex-myanmar-machine.hh(672,35): error: unused variable 'act' [-Werror,-Wunused-variable]
        unsigned int p, pe, eof, ts, te, act HB_UNUSED;
                                         ^

Which is weird since it's clearly marked HB_UNUSED. Looks like maybe HB_UNUSED is not correctly defined because of the setup? This is how we define that:

#if defined(__GNUC__) && (__GNUC__ >= 4) 
#define HB_UNUSED↦      __attribute__((unused)) 
#elif defined(_MSC_VER) /* https://github.com/harfbuzz/harfbuzz/issues/635 */ 
#define HB_UNUSED __pragma(warning(suppress: 4100 4101)) 
#else 
#define HB_UNUSED 
#endif 

[bungeman]

Hmmm.. I didn't see that one, I was looking at https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8913545807754783264/+/steps/compile__with_patch_/0/stdout

[bungeman]

Ah, I see that ARM still has the reported issue here (the need for signed char), but clang-cl isn't getting 'unused' causing Windows to have issues.

[behdad]

Ah, okay. Yeah, the -T1 doesn't change that.

[behdad]

Okay. Options are going to ragel master or to ragel 6.

I would have just reverted the upgrade. But I also want to change the Indic machine for another reason, so need a working ragel version. Let me try to see if I can get ragel 6 working.

[behdad]

Okay reverted. Let's see.