Fix runtime crash when matching tuple elements against union types

[redvers]

When pattern matching a tuple element against a union type pattern (e.g., (U8 | I128)), the codegen incorrectly loaded the unboxed primitive value as a pointer, causing a crash.

As jemc said in #4507:

the compiled code inside the match to recognize the subtle distinction
between the two different tuple shapes, and generate different code
for different tuple shape cases, with the one case we need here being
a case where we need to generated a boxed pointer for b, before w
can try to treat it as a pointer.

This new execution path is triggered when we're dealing with a TK_UNIONTYPE | TK_ISECTTYPE:

Key changes:

• In dynamic_capture_ptr(), detect union/intersection pattern types and box unboxed primitives using runtime descriptor information
• Add gendesc_size() and gendesc_fieldoffset() to access descriptor fields
• Handle 128-bit types (I128/U128) specially due to 16-byte alignment requirements - they need different offset/size calculations than smaller primitives

Fixes #4507

[ponylang-main]

Hi @redvers,

The changelog - fixed label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do.

Release notes are added by creating a uniquely named file in the .release-notes directory. We suggest you call the file 4787.md to match the number of this pull request.

The basic format of the release notes (using markdown) should be:

## Title

End user description of changes, why it's important,
problems it solves etc.

If a breaking change, make sure to include 1 or more
examples what code would look like prior to this change
and how to update it to work after this change.

Thanks.

[SeanTAllen]

This fixes a "triggers release bug".

I expect this will be merged at Tuesday's sync or shortly thereafter. I am planning on doing a release the weekend of the 31st which I think is a good window for this to go out. There will be a touch of time for it to go into a nightly and for all our projects to run against the nightly to verify no weirdness at a distance and then, out within a week. I think that fits within "triggers release".

This is a very important bug to fix. Thanks @redvers.

[jemc]

We discussed in sync.

Please use gendesc_fieldoffset instead of this code.

I would be surprised if gendesc_fieldoffset gave the wrong value for 128-bit boxed numerics. It's possible that it is wrong, but that would mean we have other problems elsewhere, and we'd want to fix those too.

[redvers]

I don't think this is possible. The issue is that we can only derive the compile_type_t of the union. The ->structure of the union is NULL, because unions don't have a (single) structure.

ie - we can't LLVMOffsetOfElement with a NULL structure.

We only have the runtime descriptor and the pointer to the raw bytes.

As far as I can tell, this is the only place in the compiler where this is the case, and why this functionality doesn't exist elsewhere.

I can't for the life of me find any other common code that can be used.

[redvers]

You know what - just because it's zero in the description table doesn't mean it has to be.
I have an idea… It's a little more invasive to elsewhere in the compiler, but it would offset all these alignment decisions to LLVM.

[redvers]

Okay - so the DESC_FIELD_OFFSET field is defined by make_field_offset:

static LLVMValueRef make_field_offset(compile_t* c, reach_type_t* t)
{
  if(t->field_count == 0)
    return LLVMConstInt(c->i32, 0, false);

I believe that primitives should have a field-count of zero? Is that a reasonable assumption?

If that is the case, two potential ways to get the work done by LLVMOffsetOfElement:

1. We can remove that hard-coded "0" in make_field_offset() when ->field_count == 0, and use this slot in the case of primitives to store the offset.

2. We can add an additional field to pony_type_t to hold that value.

The second is probably cleaner "in design" than potentially creating a different meaning for the slot depending on if it is a prmitive or not… But it involves touching a lot of places elsewhere in the compiler, as there are static definitions everywhere in the code which would require the extra field to be added.

(Would it also be a breaking change, since pony_type_t is in pony.h)?

That's the only two ways I can see to get the logic from llvm.

Help?

[redvers]

I don't need to do this math.

[nisanharamati]

I tried this today, and it works for the direct match from a concrete type to a union type, but still segfaults on match to Stringable. However, if you stagger the matches, they don't segfault (in this fix's branch. They still segfault in the 0.60.5)

actor Main
  new create(env: Env) =>
    let x: Any val = ("a", (U8(1)))
    match x
    | (let a: String, let b: Stringable) =>   // THIS SEGFAULTS
      env.out.print("(b:Stringable).string() => " + b.string())
    | (let a: String, let b: (U8 | U16)) =>  // THIS IS FIXED IN THIS PR
      match b
      | let b': Stringable =>                      // THIS DOES NOT SEGFAULT
        env.out.print("b'.string() => " + b'.string())
      end
    end

Thanks for taking this on, by the way!

[nisanharamati]

This change introduces another bug:

class val Container
  var data: Any val
  new val create(data': Any val) =>
    data = data'

primitive GetStringable
  fun val apply(p: Container): String =>
    "Container(" +
      match p.data
      | let s: Stringable => s.string()
      | let p': Container => GetStringable(p')
      else
        "Non-Stringable"
      end +
    ")"

actor Main
  let env: Env
  new create(env': Env) =>
    env = env'

    let arg = Container((
        "abcd",
        Container("1234")
      ))

    match arg.data
    | (let s: String, let p: (Container|None)) =>
      env.out.print("s is: " + s)
      env.out.print("inner is: " +
        match p
        | let p_cv: Container => GetStringable(consume p_cv)
        else
          "non-Container"
        end
      )
    else
      env.out.print("Couldn't match main Container to ({(Container)}, Container)")
    end

with

0.60.4-17b87bd [release]
Compiled with: LLVM 18.1.8 -- AppleClang-17.0.0.17000013-arm64

this produces:

s is: abcd
inner is: Container(1234)

with

0.60.4-2f2ae74a [debug]
Compiled with: LLVM 18.1.8 -- AppleClang-15.0.0.15000309-arm64
Defaults: pic=true

you get a doubled up container somehow (this really baffled me for a while before I decided to check it with another ponyc version)

s is: abcd
inner is: Container(Container(1234))

[redvers]

Thanks, I'll take a look at this after work today.
Great find!

[redvers]

What is interesting about this is that it only happens when config=release, and not when config=debug… Unexpected.

[SeanTAllen]

Almost assuredly an optimization bug @redvers. We can debug together some at office hours Monday if you want.

Sometimes you can narrow it down to a set of optimizations. Sometimes the inliner is involved and then looking at llvm is the only option.

[redvers]

So there's a secondary issue: the debug-compiled ponyc generates different codegen than the release-compiled ponyc. In the debug compiler, ast_id(pattern_type) does NOT return TK_UNIONTYPE for (Container|None) so everything is different.

I'm wondering now what other subtle differences in codegen we may have between debug and release versions.

This isn't going to be ready for tomorrow's meeting. I need to try and understand why this behaviour differs and if it's intentional or not before I try and solve this problem again.

Thank you!

[redvers]

Almost assuredly an optimization bug @redvers. We can debug together some at office hours Monday if you want.

We compared the output using both the config=debug and config=release versions with the -d flag to disable optimization. The LLVM it generated was different. I'm working my way up to try and find where it diverges.