Remove back pointer from Real to VALUE by tompng · Pull Request #344 · ruby/bigdecimal

tompng · 2025-06-08T11:03:55Z

Fixes #340
((Real*)x)->obj, a reference from DATA to VALUE, will be garbled after gc compaction. Real should not hold a reference to VALUE.

Most diffs are just a line-by-line conversion.
Introduce BDVALUE, a pair of BigDecimal VALUE and struct Real, instead of relying on VALUE reference from struct Real.
BDVALUE_OR_NIL is an nullable version of BDVALUE.

// BEFORE

Real* x = GetVpValueWithPrec(v, -1, must);
if (x != NULL) { do_something(x); return x->obj; }

// AFTER

BDVALUE x = GetVpValueWithPrec(v, -1);
do_something(x.real); return x.bigdecimal;

NULLABLE_BDVALUE y = GetVpValueWithPrecMust(v, -1);
if (y.real_or_null != NULL) {
  BDVALUE z = bdvalue_nonnullable(y);
  do_something(z.real); return z.bigdecimal;
}

This is to reduce the possibility of embedding new bugs in this pull request.

GUARD_OBJ is not so important after this change. We can remove most of them and replace it to few RB_GC_GUARD.
But in this pull request, these are left as is, to make review easier.

tompng

Most diffs are just a line-by-line conversion.
I commented where it is not a line-to-line conversion.

tompng · 2025-06-30T12:14:09Z

 {
    ENTER(1);
-    Real *p;
+    BDVALUE v;


Real *p Real *pv Real *vp is renamed to v in this pull request.
I think p represents that it's a pointer.

tompng · 2025-06-30T12:19:22Z

+bdvalue_nullable(BDVALUE v)
+{
+    return (NULLABLE_BDVALUE) { v.bigdecimal, v.real };
+}


BDVALUE NULLABLE_BDVALUE bdvalue_nonnullable bdvalue_nullable are introduced to reduce the possibility of bugs like this:

- Real *a = GetVpFooBarOrNull(x); - if (a != NULL) return Baz(a); + BDVALUE a = GetFooBarOrNull(x); + if (a != NULL) return Baz(a.real); // Should be `if (a.real != NULL)`, but hard to detect

Using NULLABLE_BDVALUE:

BDVALUE a = GetFooBarOrNull(x); // Bug. Compile error NULLABLE_BDVALUE a = GetFooBarOrNull(x); // OK if (a != NULL) return Baz(a); // Bug. Compile error if (a != NULL) return Baz(a.real); // Bug. Compile error if (a != NULL) return Baz(a.real_or_null); // Bug. We can find this bug by grep if (a.real_or_null != NULL) return Baz(a.real_or_null); // OK

The only dangerous parts are:

Where accessing unassigned BDVALUE

Where using v.real_or_null v.bigdecimal_or_nil

Where using bdvalue_nonnullable(x)

tompng · 2025-06-30T13:20:20Z

+GetBDValueWithPrecMust(VALUE v, long prec)
+{
+    return bdvalue_nonnullable(GetBDValueWithPrecInternal(v, prec, 1));
+}


Real *real = GetVpValueWithPrec(v, prec, must=1); Real *real_or_null = GetVpValueWithPrec(v, prec, must=0); Real *real = GetVpValue(v, must=1) Real *real_or_null = GetVpValue(v, must=0) // ↓ BDVALUE x = GetVpValueWithPrecMust(v, prec); NULLABLE_BDVALUE y = GetVpValueWithPrec(v, prec); BDVALUE x = GetVpValueMust(v); NULLABLE_BDVALUE y = GetVpValue(v); // x.real, x.bigdecimal: always non-null/non-nil (except unassigned case) // y.real_or_null, y.bigdecimal_or_nil

tompng · 2025-06-30T13:26:19Z

-VP_EXPORT Real *
-VpNewRbClass(size_t mx, const char *str, VALUE klass, bool strict_p, bool raise_exception)
+static NULLABLE_BDVALUE
+CreateFromString(size_t mx, const char *str, VALUE klass, bool strict_p, bool raise_exception)


VpNewRbClass, VpCreateRbObject → CreateFromString
The important part of this function is that it creates from string. The original function name doesn't represent it.
Merged these two functions into one because I couldn't think of a good name.

Good renaming.

tompng · 2025-06-30T13:28:56Z

+        b = bdvalue_nonnullable(b2);
    }

-    if (!b) return DoSomeOne(self,r,'+');


moved to else clause above

tompng · 2025-06-30T14:06:33Z

-static Real *
-rbd_reallocate_struct(Real *real, size_t const internal_digits)
+static BDVALUE
+rbd_reallocate_struct(BDVALUE value, size_t const internal_digits)


Take a pair of BigDecimal object and Real*, returns a pair of BigDecimal object and reallocated Real*

tompng · 2025-06-30T14:09:15Z

 rbd_allocate_struct_zero_limited_wrap(int sign, size_t const digits)
 {
-    return rbd_allocate_struct_zero_wrap_klass(rb_cBigDecimal, sign, digits, true);
+    return bdvalue_nonnullable(rbd_allocate_struct_zero_wrap_klass(rb_cBigDecimal, sign, digits, true));


rbd_allocate_struct_zero_wrap_klass can return null, but bigdecimal.c is/was assuming that the value is non-nullable.
This change just makes the fact clear.

tompng · 2025-06-30T14:17:43Z

+typedef struct {
+    VALUE bigdecimal;
+    Real *real;
+} BDVALUE;


BDVALUE: BigDecimal VALUE (+ Real*)
A pair of VALUE(instance of BigDecimal) and Real*.

tompng · 2025-06-30T14:19:32Z


 static VALUE
-VpCheckGetValue(Real *p)
+CheckGetValue(BDVALUE v)


Takes BDVALUE, checks exception and returns bigdecimal.

tompng · 2025-06-30T14:21:48Z

+    Real *new_real = (Real *)ruby_xrealloc(value.real, size);
    new_real->MaxPrec = internal_digits;
-    if (obj) {
-        new_real->obj = 0;


Real does not have obj field anymore. Set/Reset/Validate real->obj is removed.

mrkn

I've left two questions.

((Real*)x)->obj, a reference from DATA to VALUE, will be garbled after gc compaction. Real should not hold a reference to VALUE.

tompng force-pushed the reduce_real_obj_reverse_reference branch 4 times, most recently from d30582e to ecac757 Compare June 10, 2025 16:26

tompng force-pushed the reduce_real_obj_reverse_reference branch from ecac757 to 86a84a6 Compare June 26, 2025 15:07

tompng changed the title ~~Reduce using ((*Real)x)->obj~~ Remove back pointer from Real to VALUE Jun 26, 2025

tompng marked this pull request as ready for review June 26, 2025 15:21

tompng mentioned this pull request Jun 26, 2025

Refine SAVE and GUARD_OBJ macros #358

Closed

tompng force-pushed the reduce_real_obj_reverse_reference branch from 86a84a6 to e75cb08 Compare June 26, 2025 15:43

tompng commented Jun 30, 2025

View reviewed changes

mrkn reviewed Jul 3, 2025

View reviewed changes

Remove back pointer from Real to VALUE

489cd85

((Real*)x)->obj, a reference from DATA to VALUE, will be garbled after gc compaction. Real should not hold a reference to VALUE.

mrkn force-pushed the reduce_real_obj_reverse_reference branch from e75cb08 to 489cd85 Compare July 4, 2025 07:46

mrkn merged commit b6d2987 into ruby:master Jul 6, 2025
78 checks passed

tompng deleted the reduce_real_obj_reverse_reference branch July 6, 2025 05:07

tompng mentioned this pull request Jul 6, 2025

Fix compiling issue (when BIGDECIMAL_DEBUG is 1) #363

Merged

This was referenced Jul 28, 2025

Reduce guard obj #390

Merged

Remove ENTER and GUARD_OBJ macro #391

Merged

Uh oh!

Conversation

tompng commented Jun 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tompng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tompng Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tompng Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrkn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tompng commented Jun 8, 2025 •

edited

Loading

tompng Jun 30, 2025 •

edited

Loading

tompng Jun 30, 2025 •

edited

Loading