Project

General

Profile

Actions

Bug #18007

closed

Help developers of C extensions meet requirements in "doc/extension.rdoc"

Added by mdalessio (Mike Dalessio) over 3 years ago. Updated about 3 years ago.

Status:
Closed
Assignee:
-
Target version:
-
[ruby-core:104401]

Description

A pull request for this feature has been submitted at https://github.com/ruby/ruby/pull/4604

Problem being solved

This option is intended to help developers of C extensions to check if their code meets the requirements explained in "doc/extension.rdoc". Specifically, I want to ensure that T_DATA object classes undefine or redefine the alloc function.

https://github.com/ruby/ruby/blob/6963f8f743b42f9004a0879cd66c550f18987352/doc/extension.rdoc#label-Write+the+C+Code says:

Since Object.allocate allocates an ordinary T_OBJECT type (instead of T_DATA), it's important to either use rb_define_alloc_func() to overwrite it or rb_undef_alloc_func() to delete it.

(note: which matters when using TypedData_Make_Struct/TypedData_Wrap_Struct as the native pointer is supplied without calling the class alloc function).

There is currently no easy way for an author of a C extension to easily see where they have made the mistake of letting the default alloc function remain for their class (and therefore class.new creating a T_OBJECT instead of T_DATA and not setting the data pointer).

Description of the solution

Compiled with this option, Ruby will warn when a T_DATA object is created whose class has not undefined or redefined the alloc function.

A new function is defined, rb_data_object_check. That function is called from rb_data_object_wrap() and
rb_data_typed_object_wrap() (which implement the Data_Wrap_Struct family of macros).

The warning, when emitted, looks like this:

warning: T_DATA class Nokogiri::XML::Document should undefine or redefine the alloc function, please see doc/extension.rdoc

Examples of this problem in the wild

Using this option, I found that many of Nokogiri's classes needed to undefine allocate.

This PR also updates these core Ruby classes by undefining allocate:

  • ObjectSpace::InternalObjectWrapper
  • Socket::Ifaddr

Questions for reviewers

Does this check really need to be behind a configuration option? Performance impact is very small (see benchmarks below), but I put it behind a flag because I am worried that there may be a many C extensions that would emit warnings at runtime, and the users of those extensions cannot fix the problem and so would mostly just be annoyed.

Should this warning be emitted with the deprecated category?

Benchmarking

I benchmarked this code by allocating Nokogiri::XML::NodeSets in a loop. This is a class with a relatively simple allocate function.

The runs cover the four combinations of enabled/disabled, and warnings/no-warnings.

ruby 3.1.0dev (2021-06-25T04:02:18Z flavorjones-extens.. de943189aa) [x86_64-linux]
Warming up --------------------------------------
disabled, warn=false   490.143k i/100ms
Calculating -------------------------------------
disabled, warn=false      4.863M (± 1.5%) i/s -     49.014M in  10.081177s

ruby 3.1.0dev (2021-06-25T04:02:18Z flavorjones-extens.. de943189aa) [x86_64-linux]
Warming up --------------------------------------
 disabled, warn=true   483.070k i/100ms
Calculating -------------------------------------
 disabled, warn=true      4.839M (± 1.4%) i/s -     48.790M in  10.083899s

Comparison:
disabled, warn=false:  4863064.0 i/s
 disabled, warn=true:  4839310.1 i/s - same-ish: difference falls within error


ruby 3.1.0dev (2021-06-25T04:02:18Z flavorjones-extens.. de943189aa) [x86_64-linux]
Warming up --------------------------------------
 enabled, warn=false   484.398k i/100ms
Calculating -------------------------------------
 enabled, warn=false      4.840M (± 1.9%) i/s -     48.440M in  10.011854s

Comparison:
disabled, warn=false:  4863064.0 i/s
 enabled, warn=false:  4840123.2 i/s - same-ish: difference falls within error
 disabled, warn=true:  4839310.1 i/s - same-ish: difference falls within error


ruby 3.1.0dev (2021-06-25T04:02:18Z flavorjones-extens.. de943189aa) [x86_64-linux]
Warming up --------------------------------------
  enabled, warn=true   492.200k i/100ms
Calculating -------------------------------------
  enabled, warn=true      4.866M (± 2.1%) i/s -     48.728M in  10.017455s

Comparison:
  enabled, warn=true:  4866434.8 i/s
disabled, warn=false:  4863064.0 i/s - same-ish: difference falls within error
 enabled, warn=false:  4840123.2 i/s - same-ish: difference falls within error
 disabled, warn=true:  4839310.1 i/s - same-ish: difference falls within error

My conclusion is that the performance impact is very small, and we could omit the option if the Ruby core maintainers decide this behavior should be on by default.

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0