Feature #6083

Hide a Bignum definition

Added by Koichi Sasada about 2 years ago. Updated about 1 month ago.

[ruby-core:42891]
Status:Closed
Priority:Normal
Assignee:Yukihiro Matsumoto
Category:-
Target version:Next Major

Description

Now, the struct RBignum which is a definition of Bignum in C is located in include/ruby/ruby.h. It means we can't change implementation of Bignum. For example, using GMP as Bignum representation.

I propose to move the struct RBignum definition from include/ruby/ruby.h to bignum.c. I believe no one use struct RBignum directly (except core).

It has possibility to break binary compatibility.

hide-bignum-internal.patch Magnifier (4.66 KB) Akira Tanaka, 02/12/2014 11:45 AM

Associated revisions

Revision 44957
Added by Akira Tanaka 2 months ago

  • include/ruby/ruby.h, internal.h, ext/-test-/bignum/bigzero.c: Hide a Bignum definition. [Feature #6083]

History

#1 Updated by Yui NARUSE about 2 years ago

Binary hackers can handle C structs even if it is a hidden private struct.
So it should be enough simply to declare "struct RBignunm is not a public API".

#2 Updated by Motohiro KOSAKI about 2 years ago

Binary hackers can handle C structs even if it is a hidden private struct.
So it should be enough simply to declare "struct RBignunm is not a public API".

I disagree. As far as we consider there is backdoor, every binary
compatibility effort are
not meaningful. But I believe it clearly has a worth.

#3 Updated by Yusuke Endoh about 2 years ago

  • Status changed from Open to Assigned
  • Assignee set to Yukihiro Matsumoto

ko1 wrote:

I propose to move the struct RBignum definition from include/ruby/ruby.h to bignum.c. I believe no one use struct RBignum directly (except core).

Agreed.
I hope C API would be organized. This can be preliminary case.

naruse wrote:

Binary hackers can handle C structs even if it is a hidden private struct.
So it should be enough simply to declare "struct RBignunm is not a public API".

If you think so, it could be simpler.
README.EXT has no description about RBignum.
So we don't need to do anything.
But we can kindly do something.

Yusuke Endoh mame@tsg.ne.jp

#4 Updated by Koichi Sasada over 1 year ago

Matz, what do you think about it?

#5 Updated by Yukihiro Matsumoto over 1 year ago

  • Target version changed from 2.0.0 to next minor

I am sorry but it's too late. I admit my fault.

Matz.

#6 Updated by Motohiro KOSAKI over 1 year ago

Target version changed from 2.0.0 to next minor
I am sorry but it's too late. I admit my fault.

Matz, I don't like this decision. I believe we should choose next major
or 2.0.0. minor release should not have binary incompatibility as far
as possible.
Is this typo? If no, I'd like to propose pull it back. declaration
moving is not big task.

  • kosaki

#7 Updated by Koichi Sasada over 1 year ago

(2012/10/31 0:31), KOSAKI Motohiro wrote:

Target version changed from 2.0.0 to next minor
I am sorry but it's too late. I admit my fault.

Matz, I don't like this decision. I believe we should choose next major
or 2.0.0. minor release should not have binary incompatibility as far
as possible.
Is this typo? If no, I'd like to propose pull it back. declaration
moving is not big task.

Maybe, we don't define what is major and what is minor.

On 2.0.x, we shouldn't change binary compatibility.
But 2.1.x or later, I think it is acceptable.
(and some people don't accept :))

--
// SASADA Koichi at atdot dot net

#8 Updated by Motohiro KOSAKI over 1 year ago

On Wed, Oct 31, 2012 at 9:26 AM, SASADA Koichi ko1@atdot.net wrote:

(2012/10/31 0:31), KOSAKI Motohiro wrote:

Target version changed from 2.0.0 to next minor
I am sorry but it's too late. I admit my fault.

Matz, I don't like this decision. I believe we should choose next major
or 2.0.0. minor release should not have binary incompatibility as far
as possible.
Is this typo? If no, I'd like to propose pull it back. declaration
moving is not big task.

Maybe, we don't define what is major and what is minor.

On 2.0.x, we shouldn't change binary compatibility.
But 2.1.x or later, I think it is acceptable.
(and some people don't accept :))

Ok, please ignore my last mail then. :)

#9 Updated by Anonymous over 1 year ago

Hi,

In message "Re: Re: [ruby-trunk - Feature #6083] Hide a Bignum definition"
on Wed, 31 Oct 2012 15:31:08 +0900, KOSAKI Motohiro kosaki.motohiro@gmail.com writes:

|> Target version changed from 2.0.0 to next minor
|> I am sorry but it's too late. I admit my fault.
|
|Matz, I don't like this decision. I believe we should choose next major
|or 2.0.0. minor release should not have binary incompatibility as far
|as possible.
|Is this typo? If no, I'd like to propose pull it back. declaration
|moving is not big task.

Hiding (or no hiding) Bignum definition would not change binary
compatibility, probably you mean source compatibility?

But keeping compatibility during the release cycle is important, I
admit. So I changed my mind. If there's no big opposition, I can
accept this proposal.

                        matz.

#10 Updated by Yukihiro Matsumoto over 1 year ago

  • Target version changed from next minor to 2.0.0

#11 Updated by Yusuke Endoh over 1 year ago

  • Target version changed from 2.0.0 to Next Major

If there's no big opposition, I can accept this proposal.

Sorry, but I must voice big opposition.

Unfortunately, hiding RBignum will cause significant incompatibility.
The macro RBIGNUMBDIGITS uses RBignum internally. Many extension
libraries actually uses RBIGNUM
BDIGITS to construct a bignum object.

So, to hide RBignum, we must carefully design a new alternative API
to construct a bignum. It will take a time.
Worse, source compatibility will break anyway because it is almost
impossible to make it the same as the current RBIGNUM_BDIGITS.

(Consider replacing the internal representation with GMP.
RBIGNUM_BDIGITS must malloc a BDIGIT array and export GMP into the
array. But no one will free the allocated memory.)

Thus, I'm moving this to "Next Major". Again, I'm sorry.

Yusuke Endoh mame@tsg.ne.jp

#12 Updated by Shyouhei Urabe over 1 year ago

+1 for next major.

No one is against this basic concept of hiding Bignums, meseems. It's just a bad timing for us.

#13 Updated by Koichi Sasada over 1 year ago

(2012/11/01 11:05), shyouhei (Shyouhei Urabe) wrote:

+1 for next major.

+1 for next major or next minor.

We need discussion about how to break binary compatibility in another
thread :)

--
// SASADA Koichi at atdot dot net

#14 Updated by Yusuke Endoh over 1 year ago

2012/11/2 SASADA Koichi ko1@atdot.net:

(2012/11/01 11:05), shyouhei (Shyouhei Urabe) wrote:

+1 for next major.

+1 for next major or next minor.

We need discussion about how to break binary compatibility in another
thread :)

Note that this issue (#6083) will break source compatibility.

--
Yusuke Endoh mame@tsg.ne.jp

#15 Updated by Akira Tanaka 2 months ago

I made a patch for this issue.

It just moves struct RBignum and related macros from include/ruby/ruby.h to internal.h, not bignum.c.
It is because other files (such as gc.c, marshal.c, etc.) access internal of struct RBignum.

#16 Updated by Yukihiro Matsumoto 2 months ago

Even though this breaks source compatibility, it is worth changing it (to mark dangerous operation).
So go ahead.

Matz.

#17 Updated by Akira Tanaka 2 months ago

  • Status changed from Assigned to Closed
  • % Done changed from 0 to 100

Applied in changeset r44957.


  • include/ruby/ruby.h, internal.h, ext/-test-/bignum/bigzero.c: Hide a Bignum definition. [Feature #6083]

#18 Updated by Aaron Patterson about 1 month ago

Hi,

r44957 breaks the sqlite3 gem. I guess it uses the RBIGNUM_LEN macro:

https://github.com/sparklemotion/sqlite3-ruby/blob/2070182f461f1e2d76b9d40aa45fed2d04e9a4d6/ext/sqlite3/statement.c#L271-274

What should we use as a replacement? @Naruse, I think you added this check to the sqlite3 gem:

https://github.com/sparklemotion/sqlite3-ruby/commit/7dbd82d3

Thanks!

#19 Updated by Akira Tanaka about 1 month ago

nobu wrote a patch: https://github.com/nobu/sqlite3-ruby/compare/bignum-conversion?expand=1

I'm not sure the status to merge it to the upstream.
(I think the patch should be similified by removing code for nails != 0.)

Note that the original condition, RBIGNUMLEN(result) * SIZEOFBDIGITS <= 8, is wrong.
It tests -264 < x < 264 but it should test -263 <= x < 263 which accepts NUM2LL.

#20 Updated by Aaron Patterson about 1 month ago

Ah, thanks! I think I can get it merged upstream.

#21 Updated by Nobuyoshi Nakada about 1 month ago

I've forgotten to push the latest commit, which removes unused code for nails != 0.

Also available in: Atom PDF