Reformat the output of BIGNUMS where test cases fail. #3465

paulidale · 2017-05-14T22:56:03Z

tests are added or updated

Change the output from BIGNUM tests so they match the suggestions in #3341

# ERROR: (BIGNUM) 'a == b' failed @ test/test_test.c:403
# --- a
# +++ b
#                                                                     offset
# -  12345678901234 5678901234567890 1234567890123456 7890121234567890:  128
# -1234567890123456 7890123456789012 3456789012123456 7890123456789012:   96
# -3456789012345678 9012345678901212 3456789012345678 9012345678901234:   64
# -5678901234567890 1212345678901234 5678901234567890 1234567890123456:   32
# +                   12345678901234 5678901234567890 1234567890123456:   32
#  ^^^^^^^^^^^^^^^^ ^^                                                
# -7890121234567890 1234567890123456 7890123456789012 3456789012ffffff:    0
# +7890121234567890 1234567890123456 7890123456789012 3456789013987657:    0
#                                                              ^^^^^^^

The offset on the right is in bytes.

Extra test cases to display some of the output edge cases have been added.

paulidale · 2017-05-15T01:20:47Z

That example isn't the best. The caret highlighting is per character (i.e. digit) not per byte (pair of digits).

levitte · 2017-05-15T05:02:08Z

I'm wondering of the highlight at the beginning is really necessary when the two number have different sizes. How about this?

# ERROR: (BIGNUM) 'a == b' failed @ test/test_test.c:403
# --- a
# +++ b
#                                                                     offset
# -  12345678901234 5678901234567890 1234567890123456 7890121234567890:  128
# -1234567890123456 7890123456789012 3456789012123456 7890123456789012:   96
# -3456789012345678 9012345678901212 3456789012345678 9012345678901234:   64
# -5678901234567890 1212345678901234 5678901234567890 1234567890123456:   32
# +                   12345678901234 5678901234567890 1234567890123456:   32
# -7890121234567890 1234567890123456 7890123456789012 3456789012ffffff:    0
# +7890121234567890 1234567890123456 7890123456789012 3456789013987657:    0
#                                                              ^^^^^^^

…oth bignums have digits.

paulidale · 2017-05-15T22:54:43Z

Okay, that's an improvement.
Change committed.

dot-asm · 2017-05-16T14:09:11Z

test/test_test.c

+    int r;
+    BIGNUM *a = NULL, *b = NULL, *c = NULL, *d = NULL;
+    const char as[] = "1234567890123456789012345678901234567890123456789012"
+                     "1234567890123456789012345678901234567890123456789012"


Jagged formatting. I mean it looks like one white space is missing here and in number of other places.

Oops, missed these.

dot-asm · 2017-05-16T14:10:07Z

test/test_test.c

+    BN_hex2bn(&a, as);
+    BN_hex2bn(&b, bs);
+    BN_hex2bn(&c, cs);
+    BN_hex2bn(&d, ds);


What if either of BN_hex2bn fails?

dot-asm · 2017-05-16T14:23:02Z

How about this?

Seconded.

Change committed.

# ERROR: (BIGNUM) 'a == b' failed @ test/test_test.c:403
# --- a
# +++ b
#                                                                     offset
# -  12345678901234 5678901234567890 1234567890123456 7890121234567890:  128
# -1234567890123456 7890123456789012 3456789012123456 7890123456789012:   96
# -3456789012345678 9012345678901212 3456789012345678 9012345678901234:   64
# -5678901234567890 1212345678901234 5678901234567890 1234567890123456:   32
# +                   12345678901234 5678901234567890 1234567890123456:   32
#                                                                     
# -7890121234567890 1234567890123456 7890123456789012 3456789012ffffff:    0
# +7890121234567890 1234567890123456 7890123456789012 3456789013987657:    0
#                                                              ^^^^^^^

Empty line in the middle is unnecessary/confusing...

dot-asm · 2017-05-16T14:37:00Z

How about this?

On side note one can wonder if it makes sense to apply this even to string and memory comparisons, i.e. excess data not being marked with carets. For example right now it reads

# ERROR: (string) 'q == r' failed @ test/test_test.c:372
# --- q
# +++ r
#    0:- '1234567890klmnopqrs01234567890EFGHIJKLM0123456789XYZ'
#    0:+ '1234567890123456789012345678901234567890123456789012abcdefghijkl'
#                   ^^^^^^^^^           ^^^^^^^^^          ^^^^^^^^^^^^^^^
#   64:+ 'mnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXY+12345678901234567890123A'
#  128:+ 'BC78901234567890123456789012'

and question is if following would be more appropriate

# ERROR: (string) 'q == r' failed @ test/test_test.c:372
# --- q
# +++ r
#    0:- '1234567890klmnopqrs01234567890EFGHIJKLM0123456789XYZ'
#    0:+ '1234567890123456789012345678901234567890123456789012abcdefghijkl'
#                   ^^^^^^^^^           ^^^^^^^^^          ^^^
#   64:+ 'mnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXY+12345678901234567890123A'
#  128:+ 'BC78901234567890123456789012'

One can also wonder if following would tell the story better

# ERROR: (string) 'q == r' failed @ test/test_test.c:372
# --- q
# +++ r
#    0:- '1234567890klmnopqrs01234567890EFGHIJKLM0123456789XYZ'
#    0:+ '1234567890123456789012345678901234567890123456789012'
#                   ^^^^^^^^^           ^^^^^^^^^          ^^^
#   52:+ 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXY+123456789012'
#  116:+ '34567890123ABC78901234567890123456789012'

dot-asm · 2017-05-16T14:47:30Z

Just in case it wasn't clear, "side note" means that no modifications are expected.

dot-asm · 2017-05-16T15:03:55Z

But on related note, I don't quite see the value of # c is negative here and there...

I also wonder what do others think of comparison to NULL. More specifically if it makes sense to print any additional information besides 'b == NULL' failed. I mean right now it reads

# ERROR: (BIGNUM) 'b == NULL' failed @ test/test_test.c:405
# --- b
# +++ NULL
#                                                                     offset
# -                   12345678901234 5678901234567890 1234567890123456:   32
# -7890121234567890 1234567890123456 7890123456789012 3456789013987657:    0
# +                                                               NULL:    0
#                                                                 ^^^^

And question is ~~if it's sufficient~~ if first line alone is sufficient.

dot-asm · 2017-05-16T15:29:57Z

I also wonder what do others think of comparison to NULL.

This applies to other cases as well, i.e. not specifically to BN. On the other hand one can probably argue that it's not the presence of these methods that is "problem" but their usage. I mean it can be test programmer's choice to use TEST_ptr_op when deemed more appropriate...

levitte · 2017-05-16T16:22:38Z

I agree that the comparison with NULL doesn't need a dump. It should be clear enough from the error line,

levitte · 2017-05-16T16:26:36Z

Ah, wait... if the comparison is A == B and one of the variables turn out to have the value NULL, then it makes sense to display the content difference somehow. An immediate NULL value can be seen as a special case, and I wonder if treating it separately is worth the effort...

dot-asm · 2017-05-16T20:34:37Z

comparison to NULL

Basically what looks strange is caret marking for NULL, i.e. as if N, U, L, L were digits.

not specifically to BN.

In cases other than BN, NULL is actually not marked with carets or even dignified with offset, e,g,

# ERROR: (string) 'NULL == buf' failed @ test/test_test.c:244
# --- NULL
# +++ buf
#      - NULL
#    0:+ 'abc'

I'd say it should be same even here, i.e.

# ERROR: (BIGNUM) 'b == NULL' failed @ test/test_test.c:405
# --- b
# +++ NULL
#                                                                     offset
# -                   12345678901234 5678901234567890 1234567890123456:   32
# -7890121234567890 1234567890123456 7890123456789012 3456789013987657:    0
# +                                                               NULL

dot-asm · 2017-05-16T21:01:20Z

Regarding 'offset'. It seems that 'position' would be more appropriate term. Trouble of course is that it refers to digit position and in this case digit is nibble. But looking at nibble indices is not really handy. So it would probably be appropriate to explicitly say 'byte position' or 'bit position' (naturally multiplying byte position by 8). [As already discussed elsewhere I'd vote for latter, i.e. bit position.]

paulidale · 2017-05-16T21:52:51Z

Ah, wait... if the comparison is A == B and one of the variables turn out to have the value NULL, then it makes sense to display the content difference somehow. An immediate NULL value can be seen as a special case, and I wonder if treating it separately is worth the effort...

Exactly why it works as it does. Is trying to detect if the text version of the argument is NULL worthwhile? What about detecting (NULL) or any number of alternatives? It seems like it would be too hard to do in general but the specific case of NULL might be worthwhile. I'd argue one of the TEST_ptr functions should be used instead.

The # c is negative was an earlier attempt to handle signs which I dropped.

I think the side note about highlighting memory and string dumps should go in another ticket. It is a valid point.

paulidale · 2017-05-16T22:57:04Z

Still WIP, there is a problem where blank lines are produced which I'm looking into...

paulidale · 2017-05-16T23:47:27Z

Ready now.

levitte · 2017-05-17T05:33:29Z

Exactly why it works as it does. Is trying to detect if the text version of the argument is NULL worthwhile? What about detecting (NULL) or any number of alternatives? It seems like it would be too hard to do in general but the specific case of NULL might be worthwhile.

Nah, I think that'd be way over the top. There's only so much a framework should do.

I'd argue one of the TEST_ptr functions should be used instead.

Yup, I agree.

dot-asm · 2017-05-17T09:54:21Z

NULL should be marked with + here:

# ERROR: (BIGNUM) 'b == NULL' failed @ test/test_test.c:409
# --- b
# +++ NULL
#                                                               bit position
# -                   12345678901234 5678901234567890 1234567890123456:  256
# -7890121234567890 1234567890123456 7890123456789012 3456789013987657:    0
# -                                                               NULL

dot-asm · 2017-05-17T10:19:24Z

I find output on 32-bit systems kind of odd. Because it's forced to 7 limbs per line and as result positions end up being 224, 448, 672, 896, etc. It's cool, but I think platform-neutral output would be more appropriate. Personally I don't really care much for spaces, but injecting spaces at 64-bit boundaries can be considered as suitable for either platform. Because it's rather about [overall] output readability [and uniformity] than BN internals such as limb size. In other words I'd like to suggest to drop dependency on BN_BYTES and produce output as if it was 8.

levitte · 2017-05-17T11:26:21Z

In other words I'd like to suggest to drop dependency on BN_BYTES and produce output as if it was 8.

I second that

Fix copy paste and forget to edit typo.

paulidale · 2017-05-17T21:35:55Z

These two changes are done.

dot-asm · 2017-05-18T04:59:43Z

Something got lost in the middle. Currently it reads

# ERROR: (BIGNUM) 'a == b' failed @ test/test_test.c:407
# --- a
# +++ b
#                                                               bit position
#    12345678901234 5678901234567890 1234567890123456 7890121234567890: 1024
#  1234567890123456 7890123456789012 3456789012123456 7890123456789012:  768
#  3456789012345678 9012345678901212 3456789012345678 9012345678901234:  512
#  5678901234567890 1212345678901234 5678901234567890 1234567890123456:  256
# -7890121234567890 1234567890123456 7890123456789012 3456789012ffffff:    0
# +7890121234567890 1234567890123456 7890123456789012 3456789013987657:    0
#                                                              ^^^^^^^

while expected output is

# ERROR: (BIGNUM) 'a == b' failed @ test/test_test.c:407
# --- a
# +++ b
#                                                               bit position
# -  12345678901234 5678901234567890 1234567890123456 7890121234567890: 1024
# -1234567890123456 7890123456789012 3456789012123456 7890123456789012:  768
# -3456789012345678 9012345678901212 3456789012345678 9012345678901234:  512
# -5678901234567890 1212345678901234 5678901234567890 1234567890123456:  256
# +                   12345678901234 5678901234567890 1234567890123456:  256
# -7890121234567890 1234567890123456 7890123456789012 3456789012ffffff:    0
# +7890121234567890 1234567890123456 7890123456789012 3456789013987657:    0
#                                                              ^^^^^^^

paulidale · 2017-05-18T09:51:12Z

Drat the highlighting change. I'll figure this one out in the morning. Sleep time now.

Well caught BTW.

diff lines were not being produced properly.

paulidale · 2017-05-18T21:54:09Z

That output looks like this now:

    # ERROR: (BIGNUM) 'a == b' failed @ test/test_test.c:407
    # --- a
    # +++ b
    #                                                               bit position
    # -  12345678901234 5678901234567890 1234567890123456 7890121234567890: 1024
    # -1234567890123456 7890123456789012 3456789012123456 7890123456789012:  768
    # -3456789012345678 9012345678901212 3456789012345678 9012345678901234:  512
    # -5678901234567890 1212345678901234 5678901234567890 1234567890123456:  256
    # +                   12345678901234 5678901234567890 1234567890123456:  256
    # -7890121234567890 1234567890123456 7890123456789012 3456789012ffffff:    0
    # +7890121234567890 1234567890123456 7890123456789012 3456789013987657:    0
    #                                                              ^^^^^^^

dot-asm

Commits will be squashed to one.

dot-asm · 2017-05-19T11:28:06Z

Merged. Thanks!

Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Andy Polyakov <appro@openssl.org> (Merged from #3465)

… in openssl#3465 Don't highlight excess when comparing unequal length strings. Clean up the NULL / empty string display.

Excess bytes, when one block is longer than the other, are not explicitly highlighted. The NULL / zero length block output has been cleaned up.

Excess bytes, when one block is longer than the other, are not explicitly highlighted. The NULL / zero length block output has been cleaned up. Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3515)

… in #3465 Don't highlight excess when comparing unequal length strings. Clean up the NULL / empty string display. Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3514)

paulidale mentioned this pull request May 14, 2017

Test infrastructure items #3341

Closed

paulidale force-pushed the bn-pretty branch from e42b26e to c6558df Compare May 14, 2017 23:19

paulidale force-pushed the bn-pretty branch from c6558df to 0c1a4bb Compare May 15, 2017 04:20

Reformat the output of BIGNUMS where test cases fail.

0c1a4bb

Update the bignum output to not display the highlight carets before b…

88668e6

…oth bignums have digits.

dot-asm reviewed May 16, 2017

View reviewed changes

Update formatting and layout to be more inline with the consensus.

a4227ef

Fix blank line occasional

e2a5466

Force formatting to 8 byte boundaries for bignums.

49ffb70

Fix copy paste and forget to edit typo.

Fix problem where one bignum began with a subsequence of another and the

97b5b0b

diff lines were not being produced properly.

dot-asm approved these changes May 19, 2017

View reviewed changes

levitte approved these changes May 19, 2017

View reviewed changes

levitte pushed a commit that referenced this pull request May 19, 2017

Reformat the output of BIGNUMS where test cases fail.

ffbaf06

Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Andy Polyakov <appro@openssl.org> (Merged from #3465)

dot-asm closed this May 19, 2017

paulidale deleted the bn-pretty branch May 21, 2017 21:40

paulidale mentioned this pull request May 21, 2017

Reformat the string output diffs #3514

Closed

1 task

paulidale mentioned this pull request May 21, 2017

Reformat the memory diffs #3515

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reformat the output of BIGNUMS where test cases fail. #3465

Reformat the output of BIGNUMS where test cases fail. #3465

paulidale commented May 14, 2017

paulidale commented May 15, 2017

levitte commented May 15, 2017

paulidale commented May 15, 2017

dot-asm May 16, 2017

paulidale May 16, 2017

dot-asm May 16, 2017

dot-asm commented May 16, 2017

dot-asm commented May 16, 2017 •

edited

dot-asm commented May 16, 2017

dot-asm commented May 16, 2017 •

edited

dot-asm commented May 16, 2017 •

edited

levitte commented May 16, 2017

levitte commented May 16, 2017

dot-asm commented May 16, 2017

dot-asm commented May 16, 2017

paulidale commented May 16, 2017

paulidale commented May 16, 2017

paulidale commented May 16, 2017

levitte commented May 17, 2017

dot-asm commented May 17, 2017

dot-asm commented May 17, 2017 •

edited

levitte commented May 17, 2017

paulidale commented May 17, 2017

dot-asm commented May 18, 2017

paulidale commented May 18, 2017

paulidale commented May 18, 2017

dot-asm left a comment

dot-asm commented May 19, 2017

Reformat the output of BIGNUMS where test cases fail. #3465

Reformat the output of BIGNUMS where test cases fail. #3465

Conversation

paulidale commented May 14, 2017

paulidale commented May 15, 2017

levitte commented May 15, 2017

paulidale commented May 15, 2017

dot-asm May 16, 2017

Choose a reason for hiding this comment

paulidale May 16, 2017

Choose a reason for hiding this comment

dot-asm May 16, 2017

Choose a reason for hiding this comment

dot-asm commented May 16, 2017

dot-asm commented May 16, 2017 • edited

dot-asm commented May 16, 2017

dot-asm commented May 16, 2017 • edited

dot-asm commented May 16, 2017 • edited

levitte commented May 16, 2017

levitte commented May 16, 2017

dot-asm commented May 16, 2017

dot-asm commented May 16, 2017

paulidale commented May 16, 2017

paulidale commented May 16, 2017

paulidale commented May 16, 2017

levitte commented May 17, 2017

dot-asm commented May 17, 2017

dot-asm commented May 17, 2017 • edited

levitte commented May 17, 2017

paulidale commented May 17, 2017

dot-asm commented May 18, 2017

paulidale commented May 18, 2017

paulidale commented May 18, 2017

dot-asm left a comment

Choose a reason for hiding this comment

dot-asm commented May 19, 2017

dot-asm commented May 16, 2017 •

edited

dot-asm commented May 16, 2017 •

edited

dot-asm commented May 16, 2017 •

edited

dot-asm commented May 17, 2017 •

edited