Feature #6083: Hide a Bignum definition

• Status: Closed
• Assignee: matz (Yukihiro Matsumoto)
• Target version: 3.0

[ko1 (Koichi Sasada)]

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.

[naruse (Yui NARUSE)]

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".

[kosaki (Motohiro KOSAKI)]

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.

[mame (Yusuke Endoh)]

• Status changed from Open to Assigned
• Assignee set to matz (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 [e-mail]

[ko1 (Koichi Sasada)]

Matz, what do you think about it?

[matz (Yukihiro Matsumoto)]

• Target version changed from 2.0.0 to 2.6

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

Matz.

[kosaki (Motohiro KOSAKI)]

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

[ko1 (Koichi Sasada)]

(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

[kosaki (Motohiro KOSAKI)]

On Wed, Oct 31, 2012 at 9:26 AM, SASADA Koichi [e-mail] 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. :)

Hi,

In message "Re: [ruby-core:48640] Re: [ruby-trunk - Feature #6083] Hide a Bignum definition"
on Wed, 31 Oct 2012 15:31:08 +0900, KOSAKI Motohiro [e-mail] 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.

[matz (Yukihiro Matsumoto)]

• Target version changed from 2.6 to 2.0.0

[mame (Yusuke Endoh)]

• Target version changed from 2.0.0 to 3.0

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 [e-mail]

[shyouhei (Shyouhei Urabe)]

+1 for next major.

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

[ko1 (Koichi Sasada)]

(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

[mame (Yusuke Endoh)]

2012/11/2 SASADA Koichi [e-mail]:

(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 [e-mail]

[akr (Akira Tanaka)]

• File hide-bignum-internal.patch hide-bignum-internal.patch added

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.

[matz (Yukihiro Matsumoto)]

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

Matz.

[akr (Akira Tanaka)]

• 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.
  [ruby-core:42891] [Feature #6083]

[tenderlovemaking (Aaron Patterson)]

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 (Yui NARUSE), I think you added this check to the sqlite3 gem:

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

Thanks!

[akr (Akira Tanaka)]

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 -264 < x < 264 but it should test -263 <= x < 263 which accepts NUM2LL.

[tenderlovemaking (Aaron Patterson)]

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

[nobu (Nobuyoshi Nakada)]

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