Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework ordinals #10348

Closed
wants to merge 8 commits into from
Closed

Rework ordinals #10348

wants to merge 8 commits into from

Conversation

@levitte
Copy link
Member

levitte commented Nov 4, 2019

The ordinals file are tedious to handle when there are merge conflicts, with all the renumbering that always has to happen.

During the last OpenSSL face2face, this was an item of discussion, and we concluded that during development, these fixed positions aren't important, as we don't guarantee in any way that a symbol location in a transfer vector stays the same. However, as soon as features are essentially frozen, i.e. in beta or final releases, these numbers are important, at least on operating systems where the location in a transfer vector is still important for shared libraries and the like.

So we introduce a couple of markers for symbols that haven't been assigned a number yet:

?   signifying it gets the previous symbol's number plus one
?+  signifying it gets the same number as the previous symbol

That means that for example, one of the last symbols in util/libssl.num, which has this line:

OSSL_default_ciphersuites               509    3_0_0   EXIST::FUNCTION:

is changed to:

OSSL_default_ciphersuites               ?      3_0_0   EXIST::FUNCTION:

At beta or final release, whichever comes first (for 3.0, that's going to be the first beta), such symbols will be assigned a fixed number again, which happens to be suitable at the time.

@levitte levitte requested a review from mattcaswell Nov 4, 2019
levitte added 7 commits Nov 4, 2019
Symbols that have appeared since 1.1.1 was released are considered
unassigned in the development branch.   This is marked by having a
question mark as its ordinal number.

This introduces two new markers to be used instead of ordinal numbers:

    ?   signifying it gets the previous symbol's number plus one
    ?+  signifying it gets the same number as the previous symbol

'?+' should remain rare, but is useful to create aliases when needed
(for example when two different symbols clash because they only differ
in character case, see include/openssl/symhacks.h)

The intention is that a development branch won't have set numbers for
new symbols, and that the final numbers will only get allocated when
making beta or final releases.
We preserve the number or '?' or '?+', but assign numbers internally
on the latter, to ensure we keep the order of the input.
…mbols

This should be used when it's time to assign constant numbers to the
unassigned symbols.
…syms

If a script wants to display how many symbols have assigned numbers
and how many don't, this gives them those numbers.
When the source isn't in development any more (the version number
doesn't the tags 'dev' or 'alpha'), we renumber the unassigned symbols
to ensure that we have fixed numbers on all.
@levitte levitte force-pushed the levitte:rework-ordinals branch from 636c460 to 035d311 Nov 4, 2019
Copy link
Contributor

paulidale left a comment

Was the idea of sorting the ordinals alphabetically also suggested to reduce the number of merge conflicts further?

Adding new items to the end of the .num file will still result in conflicts. They'll be easier to fix at least.

@levitte

This comment has been minimized.

Copy link
Member Author

levitte commented Nov 4, 2019

I don't recall that, and chances are that it won't reduce conflicts that much.
Maybe in another iteration, but that's much harder to deal with.

@levitte levitte dismissed paulidale’s stale review Nov 4, 2019

I forgot to fix the test recipe

@levitte levitte changed the title Rework ordinals WIP: Rework ordinals Nov 4, 2019
@levitte

This comment has been minimized.

Copy link
Member Author

levitte commented Nov 4, 2019

Test added

@mattcaswell

This comment has been minimized.

Copy link
Member

mattcaswell commented Nov 4, 2019

I don't recall that, and chances are that it won't reduce conflicts that much.
Maybe in another iteration, but that's much harder to deal with.

Yes, that was discussed. My guess is that will reduce conflicts significantly.

@levitte levitte changed the title WIP: Rework ordinals Rework ordinals Nov 4, 2019
@levitte

This comment has been minimized.

Copy link
Member Author

levitte commented Nov 4, 2019

How about we start with this? Meanwhile, I'll have another think on the alphabetical sort (and if I understand correctly, only of the symbols that are yet unassigned, right?), and save that for another PR.

@paulidale

This comment has been minimized.

Copy link
Contributor

paulidale commented Nov 4, 2019

Why just the new symbols? Sorting is cheap and the more that is sorted, the fewer the collisions.

Copy link
Member

mattcaswell left a comment

I'm fine with this as-is. I don't think it will reduce any conflicts though until we also add sorting....but I'm ok with leaving that until another PR.

@mattcaswell

This comment has been minimized.

Copy link
Member

mattcaswell commented Nov 4, 2019

Why just the new symbols? Sorting is cheap and the more that is sorted, the fewer the collisions.

I tend to agree with @paulidale here.

@levitte

This comment has been minimized.

Copy link
Member Author

levitte commented Nov 4, 2019

Ohhhhhhh.... While preserving numbers, right? A little like we do with error strings, yeah?

Yeahok, I see how that would work, with just one exception; aliases, where there are needed, will be quite hard to deal with. The ?+ marker assumes attachment to the previous line (which is presumably marked with ?), and that assumption doesn't hold after a shuffle.
(aliases are necessary for the rare case where we have symbols that differ from others only by casing, see include/openssl/symhacks.h for the gory details)

But, other PR

@mattcaswell

This comment has been minimized.

Copy link
Member

mattcaswell commented Nov 4, 2019

Ohhhhhhh.... While preserving numbers, right?

Right!

@richsalz

This comment has been minimized.

Copy link
Contributor

richsalz commented Nov 4, 2019

I am all in favor of sorting. I suggest =otherName for "aliases" so that they are not dependant on the other.

@levitte

This comment has been minimized.

Copy link
Member Author

levitte commented Nov 4, 2019

I am all in favor of sorting. I suggest =otherName for "aliases" so that they are not dependant on the other.

That... was a workable idea!

openssl-machine pushed a commit that referenced this pull request Nov 5, 2019
Symbols that have appeared since 1.1.1 was released are considered
unassigned in the development branch.   This is marked by having a
question mark as its ordinal number.

This introduces two new markers to be used instead of ordinal numbers:

    ?   signifying it gets the previous symbol's number plus one
    ?+  signifying it gets the same number as the previous symbol

'?+' should remain rare, but is useful to create aliases when needed
(for example when two different symbols clash because they only differ
in character case, see include/openssl/symhacks.h)

The intention is that a development branch won't have set numbers for
new symbols, and that the final numbers will only get allocated when
making beta or final releases.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #10348)
openssl-machine pushed a commit that referenced this pull request Nov 5, 2019
We preserve the number or '?' or '?+', but assign numbers internally
on the latter, to ensure we keep the order of the input.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #10348)
openssl-machine pushed a commit that referenced this pull request Nov 5, 2019
…mbols

This should be used when it's time to assign constant numbers to the
unassigned symbols.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #10348)
openssl-machine pushed a commit that referenced this pull request Nov 5, 2019
…syms

If a script wants to display how many symbols have assigned numbers
and how many don't, this gives them those numbers.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #10348)
openssl-machine pushed a commit that referenced this pull request Nov 5, 2019
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #10348)
openssl-machine pushed a commit that referenced this pull request Nov 5, 2019
When the source isn't in development any more (the version number
doesn't the tags 'dev' or 'alpha'), we renumber the unassigned symbols
to ensure that we have fixed numbers on all.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #10348)
openssl-machine pushed a commit that referenced this pull request Nov 5, 2019
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #10348)
openssl-machine pushed a commit that referenced this pull request Nov 5, 2019
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #10348)
@levitte

This comment has been minimized.

Copy link
Member Author

levitte commented Nov 5, 2019

Merged.

5d61758 util/*.num: deassign ordinal numbers from new symbols
3da95f3 OpenSSL::Ordinals: Handle symbols with unassigned ordinal numbers
81ddd95 OpenSSL::Ordinals: add a renumber() function, to assign unassigned symbols
a4aab78 OpenSSL::Ordinals: when validating, collect statistics on unassigned syms
e4f2d53 util/mkdef.pl: writer_VMS(): handle symbols with no assigned number
8635730 util/mknum.pl: Call OpenSSL::Ordinals::renumber() for real releases
b6fc662 util/mknum.pl: output stats on unassigned symbols
6af1b11 test/recipes/02-test_ordinals.t: Take '?' and '?+' into account

@levitte levitte closed this Nov 5, 2019
@kaduk

This comment has been minimized.

Copy link
Contributor

kaduk commented Nov 12, 2019

I confess I'm still a little confused at when ?+ would be used; is there a simple-to-digest example?

@levitte

This comment has been minimized.

Copy link
Member Author

levitte commented Nov 12, 2019

It's for symbols that are aliases for other symbols. Nowadays, it's all derived from include/openssl/symhacks.h. If you take any of the symbols there and do a case insensitive grep, you might seen why those hacks are there at all, at least if you bear in mind that on some systems, symbol names are normally viewed without regard for case.

In util/libcrypto.num, this translates into symbols with the same ordinal number (whenever they get one assigned)

@levitte

This comment has been minimized.

Copy link
Member Author

levitte commented Nov 12, 2019

We have become much better lately at not having symbols that only differ by case, so I hope that some day, symhacks.h will simply go away.

@DDvO

This comment has been minimized.

Copy link
Contributor

DDvO commented Nov 14, 2019

@Akretsch and myself are very glad about this pragmatic simplification!
In particular when a PR that adds items to libcrypto.num needs to be rebased several times over a longer period, handling spurious merge conflicts was a PITA. This burden is gone now :)

@mattcaswell

This comment has been minimized.

Copy link
Member

mattcaswell commented Nov 14, 2019

In particular when a PR that adds items to libcrypto.num needs to be rebased several times over a longer period, handling spurious merge conflicts was a PITA. This burden is gone now :)

Unfortunately its not enough by itself to avoid constant rebases :-(
It is a good step towards that, but we additionally need the .num files to be sorted alphabetically instead of numerically to finish the job. That's a bit trickier though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.