[makeotfexe] Increase code coverage #1008

khaledhosny · 2019-10-18T18:56:09Z

These tests increase code coverage of feat.c, featgram.c, and featscan.c to nearly 80% (these three files, I believe, is what parses feature files and handles their syntax). with most of the remaining uncovered code being unreachable code (will add more tests once #1006 is fixed).

Opening this to get early review since I have been working on it for a few weeks already.

Fixes #781.

cjchapman · 2019-10-18T19:55:45Z

@khaledhosny You've got some tests failing in CI. Let me know if you need help fixing them, and if not, then let me know when you're done fixing them.

khaledhosny · 2019-10-18T21:01:28Z

4 of the failing issues are testing the code below (and a similar block in the same function):

afdko/c/makeotf/makeotf_lib/source/hotconv/featgram.c

Lines 479 to 480 in 414ef3b

    
           if (m.ulval > ((unsigned long)0xFFFFFFFF)) 
        
               zzerr("not in range 0 .. ((1<<32) -1)");

But for some reason this check is not working on Windows and out of boundary values are accepted, I wonder if the size of unsigned long on Windows is to blame here.

cjchapman · 2019-10-18T21:03:47Z

@khaledhosny I've been changing other code in the AFDKO to use uint32_t instead of unsigned long to deal with issues like that.

cjchapman · 2019-10-18T21:07:13Z

@khaledhosny In general, I've been trying to move the code over to using explicit bit sizes via:

uint8_t
int8_t
uint16_t
int16_t
uint32_t
int32_t
uint64_t
int64_t

khaledhosny · 2019-10-18T21:19:08Z

@khaledhosny I've been changing other code in the AFDKO to use uint32_t instead of unsigned long to deal with issues like that.

That is the right thing to do. In this case I think we need 64bit integers though since the code is storing values that don’t find in 32bit and we don’t want unsigned 32bit to silently warp around, so I used int64_t and uint64_t. Lets see if it fixes the issue.

khaledhosny · 2019-10-18T21:30:24Z

Hmm, this fixed one test but not the other 3 which does not make sense. I think I’ll need to debug this on a Windows machine. @cjchapman I appreciate if you can help with this, since I haven’t done AFDKO development on Windows before and not even sure I will be able to have a working setup.

cjchapman · 2019-10-18T23:41:15Z

@khaledhosny OK, I have a Windows VM I can use for debugging. I won't be able to get it until Monday, at the earliest.

khaledhosny · 2019-10-18T23:42:33Z

I spent 2 hours trying to get AFDKO to build on Windows with no luck. I might give it another try tomorrow.

cjchapman · 2019-10-18T23:50:57Z

OK.

The code is supposed to fix dflt script into DFLT, but it actually kept it dflt which would then lead to empty script in the table output.

These lines will never be reached since featMsg(hotERROR, ...) will longjmp out of the function.

The lookup is not defined here not already define.

The tests with `.bad` suffix are files with errors and should fail to be processed, the other files are good files and have expected output to compare against. It is assumed that the current makeotfexe output is correct.

Use explicit int64_t and uint64_t instead of platform-dependent long and unsigned long so that code that stores values needing more than 32bit works on Windows as well.

khaledhosny · 2019-10-19T23:51:53Z

I managed to get Windows built setup, ran the tests and, surprisingly enough, none failed!

cjchapman · 2019-10-20T00:33:31Z

@khaledhosny I wish that were surprising for me. We've seen that before with other tools that had memory bugs, especially uninitialized memory bugs. Try running the test with address sanitizer (ASAN) on Mac or Linux (or Valgrind on Linux, or I think there's some Visual Studio equivalent for Windows), that might help. If not, I can dig into it further next week (Monday at the earliest).

khaledhosny · 2019-10-21T23:42:00Z

tests/makeotfexe_data/input/spec/9f-3.fea

@@ -7,5 +7,5 @@ table OS/2 {
 	       67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87
               88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106
               107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122
-               123 124 125 126 127 128;
+               123 124 125 126 127;


The test is supposed to check that extra bits are gracefully ignored.

Well, it was triggering a buffer overflow. When I fixed the buffer overflow, it was complaining that the ; was not found.

When I didn't see "bad" in the name, I incorrectly assumed that you meant it to be "good". 😀

So... the extra bits are not gracefully ignored before or after my buffer overflow check. I'm already not crazy about the fact that I hand-edited what's supposed to be ANTLR-generated code, but I wanted to prevent the memory bug from occurring.

I'll leave it to you how you want to handle this from the test side. At least now we know why one of the four CI failures was occurring.

Just to be clear: I'm fine with debugging issues in any part of the code we plan to keep (i.e. the human-written parts of the code). I just don't want to spend too much time debugging issues in the ANTLR-generated parts of the code that we plan to replace (i.e. with something generated by a newer tool).

The problems don‘t seem to be in the generated code, the grammar file featgam.g has many C snippets and these seem to be the culprit. It is rather too late here for me to think straight though, so I‘ll check your finding later.

@khaledhosny OK. I dug into this a bit further. I'm seeing the value set to 0x000000007fffffff by the strtol() call in line 1050 below:

afdko/c/makeotf/makeotf_lib/source/hotconv/feat.c

Lines 1048 to 1051 in 793a698

void zzcr_attr(Attrib *attr, int type, char *text) {

if (type == T_NUM) {

attr->lval = strtol(text, NULL, 10);

} else if (type == T_NUMEXT) {

Perhaps you could replace the strtol with strtoll, if strtoll is portable.

I went ahead and replaced strtol with strtoll in that zzcr_attr function. That seems to have fixed the remaining three Windows CI issues.

I noticed some other strtol in that file that I did not change, so if you run into any more cases of a value getting capped at 0x000000007fffffff on Windows, then that's a good place to start looking.

@khaledhosny I just pushed a better fix for 9f-3.fea in which the code just ignores the extra bits in the UnicodeRange. Note that I hand-edited both featgram.g and featgram.c as I don't have antlr built on my system, so please check that you can still compile featgram.g to featgram.c.

cjchapman · 2019-10-22T14:38:10Z

Apparently no one has built antlr in the past two years. 🤦‍♂

cjchapman · 2019-10-22T14:45:54Z

I tried compiling featgram.g to featgram.c with antlr and then cleaning it up with clang-format. Miraculously, I get the same code with just minor line break & white space differences.

khaledhosny · 2019-10-22T21:17:26Z

Apparently no one has built antlr in the past two years. 🤦‍♂

I did two days earlier, it works pretty well actually (the minor edits required are even described in c/makeotf/makeotf_lib/build/hotpccts/readme.txt and there is even a makefile.

Use the makefile under c/makeotf/makeotf_lib/build/hotpccts/, keeping the manual edits to fix cpplint issues.

cjchapman · 2019-10-22T21:23:12Z

Apparently no one has built antlr in the past two years. 🤦‍♂

I did to days earlier, it works pretty well actually (the minor edits required are even described in c/makeotf/makeotf_lib/build/hotpccts/readme.txt and there is even a makefile.

Did you just not check in the fix to err.h then? I couldn't build it until I fixed that.

khaledhosny · 2019-10-22T21:24:56Z

Apparently no one has built antlr in the past two years. 🤦‍♂

I did to days earlier, it works pretty well actually (the minor edits required are even described in c/makeotf/makeotf_lib/build/hotpccts/readme.txt and there is even a makefile.

Did you just not check in the fix to err.h then? I couldn't build it until I fixed that.

I had to fix some file to get it to compile indeed, but forgot to commit it, and it seems to be the same fix you committed.

miguelsousa · 2019-10-22T23:13:07Z

@cjchapman @khaledhosny I've restarted Azure but the macOS job is still failing the same way. Comparing with the last commit that passed, I see that the wheel's tags of yesterday's job are macosx-10.13-x86_64 whereas today's are macosx-10.14-x86_64.

Our Azure config specifies 10.13 and the job seems to use a 10.13 image indeed, so I don't quite understand why the wheel ends up referencing 10.14.

josh-hadley · 2019-10-23T19:34:32Z

Regarding the Azure build failures, looking at the Mac "Build wheel, run tests, and uninstall" step that is failing, one difference that I see is that the new (failing) ones use runner == 2.159.2 whereas the older ones use 2.158.0 (compare ~line 16 of the build logs). Maybe that's something to do with the failure (environment variables)? Not sure if that can be configured anywhere in the Azure scheme.

miguelsousa · 2019-10-23T20:09:35Z

I'd say let's go ahead and merge it. The Azure CI is just a backup anyway. Perhaps they broke it in the process of making a 10.15 (Catalina) image available.

khaledhosny · 2019-10-23T20:36:08Z

I still want to add a few more tests. but we can merge this now and open a new PR later.

miguelsousa · 2019-10-23T20:37:01Z

@khaledhosny no, keep going. I thought this was done.

markedOK is never false here.

khaledhosny · 2019-10-26T01:07:05Z

I think I’m done, no more tests to add.

cjchapman · 2019-10-26T01:31:51Z

@readroberts Would you please review this PR? You know this code much better than I do. Also, I made some of the changes and suggested some of the others, so I think I should recuse myself from reviewing for those reasons as well.

readroberts

I have reviewed all the code changes. Looks good to me.

khaledhosny requested a review from cjchapman October 18, 2019 18:56

khaledhosny added 7 commits October 19, 2019 21:09

[makeotfexe] Fix typo

e799119

The code is supposed to fix dflt script into DFLT, but it actually kept it dflt which would then lead to empty script in the table output.

[makeotfexe] setVendId_str() always returns 0

807e0ff

[makeotfexe] Redundant returns

251e38c

These lines will never be reached since featMsg(hotERROR, ...) will longjmp out of the function.

[makeoefexe] Fix misleading error message

029d3d3

The lookup is not defined here not already define.

[makeotfexe] Unused function

ae07589

[tests] Start adding fea spec tests

fd71079

The tests with `.bad` suffix are files with errors and should fail to be processed, the other files are good files and have expected output to compare against. It is assumed that the current makeotfexe output is correct.

[makeotfexe] Blind fix to Windows CI failure

333f4b3

Use explicit int64_t and uint64_t instead of platform-dependent long and unsigned long so that code that stores values needing more than 32bit works on Windows as well.

khaledhosny force-pushed the khaled-makeotfexe-fea-spec branch from f80aa71 to 333f4b3 Compare October 19, 2019 19:09

cjchapman added 2 commits October 21, 2019 16:38

[makeotfexe] check for array overflow when processing UnicodeRange

983ea90

[makeotfexe] corrected UnicodeRange data to end with 127 in 9f-3.fea

793a698

cjchapman force-pushed the khaled-makeotfexe-fea-spec branch from 0e0793c to 793a698 Compare October 21, 2019 23:38

khaledhosny commented Oct 21, 2019

View reviewed changes

cjchapman added 3 commits October 21, 2019 20:22

[makeotfexe] use strtoll instead of strtol in zzcr_attr

cf785fd

[makeotfexe] better fix for 9f-3.fea: ignore extra bits in UnicodeRange

ea14048

[makeotfexe] fixed a bug in err.h that prevented compilation of antlr

6882fb5

[makeotfexe] Update ANTLR generated files

b4e2209

Use the makefile under c/makeotf/makeotf_lib/build/hotpccts/, keeping the manual edits to fix cpplint issues.

khaledhosny marked this pull request as ready for review October 23, 2019 20:35

khaledhosny added 2 commits October 26, 2019 01:19

[makeotfexe] Remove some dead code

db82b78

markedOK is never false here.

[tests] Add few more feature file tests

4693bfc

cjchapman requested review from readroberts and removed request for cjchapman October 26, 2019 01:29

readroberts approved these changes Oct 30, 2019

View reviewed changes

cjchapman merged commit df3768e into develop Oct 31, 2019

cjchapman deleted the khaled-makeotfexe-fea-spec branch October 31, 2019 17:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[makeotfexe] Increase code coverage #1008

[makeotfexe] Increase code coverage #1008

khaledhosny commented Oct 18, 2019 •

edited

Loading

cjchapman commented Oct 18, 2019

khaledhosny commented Oct 18, 2019

cjchapman commented Oct 18, 2019

cjchapman commented Oct 18, 2019

khaledhosny commented Oct 18, 2019

khaledhosny commented Oct 18, 2019

cjchapman commented Oct 18, 2019

khaledhosny commented Oct 18, 2019

cjchapman commented Oct 18, 2019

khaledhosny commented Oct 19, 2019

cjchapman commented Oct 20, 2019

khaledhosny Oct 21, 2019

cjchapman Oct 21, 2019

cjchapman Oct 21, 2019

cjchapman Oct 21, 2019

cjchapman Oct 21, 2019

cjchapman Oct 22, 2019

khaledhosny Oct 22, 2019

cjchapman Oct 22, 2019

cjchapman Oct 22, 2019

cjchapman Oct 22, 2019

cjchapman commented Oct 22, 2019

cjchapman commented Oct 22, 2019

khaledhosny commented Oct 22, 2019 •

edited

Loading

cjchapman commented Oct 22, 2019

khaledhosny commented Oct 22, 2019

miguelsousa commented Oct 22, 2019

josh-hadley commented Oct 23, 2019 •

edited

Loading

miguelsousa commented Oct 23, 2019

khaledhosny commented Oct 23, 2019

miguelsousa commented Oct 23, 2019

khaledhosny commented Oct 26, 2019

cjchapman commented Oct 26, 2019

readroberts left a comment

	void zzcr_attr(Attrib attr, int type, char text) {
	if (type == T_NUM) {
	attr->lval = strtol(text, NULL, 10);
	} else if (type == T_NUMEXT) {

[makeotfexe] Increase code coverage #1008

[makeotfexe] Increase code coverage #1008

Conversation

khaledhosny commented Oct 18, 2019 • edited Loading

cjchapman commented Oct 18, 2019

khaledhosny commented Oct 18, 2019

cjchapman commented Oct 18, 2019

cjchapman commented Oct 18, 2019

khaledhosny commented Oct 18, 2019

khaledhosny commented Oct 18, 2019

cjchapman commented Oct 18, 2019

khaledhosny commented Oct 18, 2019

cjchapman commented Oct 18, 2019

khaledhosny commented Oct 19, 2019

cjchapman commented Oct 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjchapman commented Oct 22, 2019

cjchapman commented Oct 22, 2019

khaledhosny commented Oct 22, 2019 • edited Loading

cjchapman commented Oct 22, 2019

khaledhosny commented Oct 22, 2019

miguelsousa commented Oct 22, 2019

josh-hadley commented Oct 23, 2019 • edited Loading

miguelsousa commented Oct 23, 2019

khaledhosny commented Oct 23, 2019

miguelsousa commented Oct 23, 2019

khaledhosny commented Oct 26, 2019

cjchapman commented Oct 26, 2019

readroberts left a comment

Choose a reason for hiding this comment

khaledhosny commented Oct 18, 2019 •

edited

Loading

khaledhosny commented Oct 22, 2019 •

edited

Loading

josh-hadley commented Oct 23, 2019 •

edited

Loading