Feature #6083

Hide a Bignum definition

Added by Koichi Sasada over 3 years ago. Updated over 1 year ago.

[ruby-core:42891]
Status:Closed
Priority:Normal
Assignee:Yukihiro Matsumoto

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 over 1 year ago

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

Revision 44957
Added by Akira Tanaka over 1 year ago

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

History

#1 Updated by Yui NARUSE over 3 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 over 3 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 over 3 years ago

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

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 about 3 years ago

Matz, what do you think about it?

#5 Updated by Yukihiro Matsumoto almost 3 years 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 almost 3 years 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 almost 3 years 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 almost 3 years 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 2 years 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 2 years ago

  • Target version changed from next minor to 2.0.0

#11 Updated by Yusuke Endoh over 2 years 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 RBIGNUM_BDIGITS 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 2 years 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 2 years 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 2 years 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 over 1 year 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 over 1 year 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 over 1 year ago

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

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 over 1 year 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 over 1 year 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, RBIGNUM_LEN(result) * SIZEOF_BDIGITS <= 8, is wrong.
It tests -2*64 < x < 264 but it should test -263 <= x < 2*63 which accepts NUM2LL.

#20 Updated by Aaron Patterson over 1 year ago

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

#21 Updated by Nobuyoshi Nakada over 1 year ago

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

Also available in: Atom PDF