Include JIT information in crash reports

[chrisseaton]

Issues fixed by this PR:

• logic was duplicated
• users in error.c weren't getting the JIT information
• the macro was (accidentally?) exposed in the header

I'm not intending to expose rb_description() as new public API - have I written the header correctly therefore?

Was the macro RUBY_DESCRIPTION_WITH previously public API? Why and should I keep it as public?

Motivation for this change is adding more option flags in the description string, such as which GC is in use.

[chrisseaton]

Context is we want to do this next chrisseaton@38b904d.

[XrXr]

It seems that it should be possible to select between two string literals like the old code and avoid the need to use strlcat. Is there some particular reason you picked this approach?

[chrisseaton]

The context here is I'm trying to make it possible to add as many of these flags as we want. So three possible combinations is easy, but if I had another flag, independent of the two JIT flags, now I need six literals, etc.

The particular flag I want to add is +MMTk for the GC we're integrating.

I think these flags are very useful for these kind of major features, which completely change how you're debugging a crash or something, which is why we want them so prominent in the description and version string.

[XrXr]

Ah okay, since the badges are infix some string building seems unavoidable. I think you could make this function shorter by picking between an empty string and the +... string for each badge and ending with a single call to snprintf. Bonus for avoiding the non ISO strlcat. You might also want to switch to a statically sized buffer instead of calling malloc. A size computed by summing sizeof(...) should be suitable for sizing a static character array.

[XrXr]

Also, since MMTK is not part of Ruby (yet), I'm tempted to keep the current simpler setup. Since MMTK will need to patch this function anyways maybe the string complexity should be in that patch instead of in this repo.

[chrisseaton]

@XrXr could I get you to re-review and re-consider merging without MMTk? In getting all the tests passing I've realised it's got a benefit right now - the crash reporter currently uses the default description, without the flags. That's because I guess they don't want to run any logic while handling a crash. But my approach runs the logic up-front to change the stored description, so a crash after a certain point will get the flags without having to run any logic.

Before:

% built/bin/ruby --jit -e 'Process.kill :SEGV, $$'
-e:1: [BUG] Segmentation fault at 0x000000013000c000
ruby 3.2.0dev (2022-06-14T14:23:13Z master 9f09397bfe) [arm64-darwin21]

After:

% built/bin/ruby --jit -e 'Process.kill :SEGV, $$'
-e:1: [BUG] Segmentation fault at 0x0000000131f44010
ruby 3.2.0dev (2022-06-15T13:37:23Z description dd83f17d17) +MJIT [arm64-darwin21]

Seems like a crash is exactly when you want to see these flags!

I also simplified the logic as you suggested to use snprintf.

[XrXr]

I think it's useful to have +YJIT in crash reports whenever appropriate. It could be inferred from the process memory map, but it doesn't hurt to be clearer.

 error.c                                      |  8 ++++----
 include/ruby/version.h                       |  3 ++-
 ruby.c                                       |  8 +++++---
 test/-ext-/bug_reporter/test_bug_reporter.rb |  2 --
 test/ruby/test_rubyoptions.rb                |  2 +-
 version.c                                    | 37 +++++++++++++++++--------------------
 6 files changed, 29 insertions(+), 31 deletions(-)

The patch deletes more line than adds, so I think this change pays for itself in terms of complexity.

[chrisseaton]

Ok that still de-duplicates the logic for choosing between descriptions so will be easier to merge what I want with MMTk.

[nobu]

Suggested change

	VALUE description = rb_obj_freeze(rb_usascii_str_new_cstr(ruby_dynamic_description));
	const char *desc = ruby_dynamic_description;
	VALUE description = rb_obj_freeze(rb_usascii_str_new_static(desc, (long)strlen(desc)));

[nobu]

Why needs to separate ruby_init_dynamic_description from Init_ruby_description?
Aren't the following change and moving Init_ruby_description call to the place ruby_init_dynamic_description is called in this PR suffice?

void
Init_ruby_description(void)
{
    VALUE description;

    if (MJIT_OPTS_ON) {
        ruby_dynamic_description = ruby_description_with_mjit;
        description = MKSTR(description_with_mjit);
    }
    else if (rb_yjit_enabled_p()) {
        ruby_dynamic_description = ruby_description_with_yjit;
        description = MKSTR(description_with_yjit);
    }
    else {
        description = MKSTR(description);
    }

    /*
     * The full ruby version string, like <tt>ruby -v</tt> prints
     */
    rb_define_global_const("RUBY_DESCRIPTION", /* MKSTR(description) */ description);
}

[chrisseaton]

I left definition of RUBY_DESCRIPTION where it was, but I wanted to create the dynamic description early, so that it's set before --version is processed.

[nobu]

I don't think it is a significant difference if the constant is defined earlier a little.

[XrXr]

It might be better If we pass whether MJIT is on into Init_ruby_description() rather than going through a global but that's probably off topic.