fix interp: unknown type kind in markExternalValue when compiling libsecp256k1

[hectorchu]

Fixes #5292

[deadprogram]

Hello @hectorchu thanks for the PR. It would be great if you could add a basic test that shows this is working as expected. What do you think?

[hectorchu]

I'd love to but I had a look at the testdata and I don't really speak LLVM. I just changed what I needed to to get my C library to compile. I have no idea how to write a unit test for this.

[deadprogram]

From looking into this a little more, seems like the VectorTypeKind handling is incorrect. It reuses array APIs that don't apply to vectors. It likely works for the specific libsecp256k1 case by accident (e.g., the vector values might be constant and the extract operations happen to go through), but it's not a correct general solution.

A proper fix for vectors would need to use CreateExtractElement with an index value, or more practically for the interp package to treat vectors like integers/floats (since SIMD vectors in embedded contexts rarely contain pointers and could just be skipped). You'd also want to confirm what vector types libsecp256k1 actually produces and verify the behavior is correct.

The implementation should probable keep the vector case separated from the array case, rather than using fallthrough, and either properly iterate with CreateExtractElement or skip the vector as a non-pointer type (like integers).

[hectorchu]

Yes, so when I ran this through ChatGPT earlier this week, it said much of the same things. I did compare results between my simple change, and a change suggested by GPT which included CreateExtractElement, VectorSize and so on. First, ArraySize and VectorSize are the same. Then I printed the elements that were extracted, and they matched whether using plain array API or the general API. The values that I printed were not constant either, some were very big and others very small.

I see, I could just skip processing of Vectors, seeing that they did contain i32/64s. Would you like me to make that change?

Without really wanting to "dive" into the libsecp256k1 (I'm not sure how clang is vectorizing the code), I did confirm that all the functionality I required from it was working a-ok.

[deadprogram]

I see, I could just skip processing of Vectors, seeing that they did contain i32/64s. Would you like me to make that change?

Looking at it again, I wonder if that would be the correct change. The elements would not be marked, and that could result in undefined behavior. It would better to fully implement the vector type, and that would require some associated tests.

[hectorchu]

I probably will be unable to contribute further to this PR then. I think the author of this code @aykevl should just do a quick change given he probably knows exactly what to do here. It would be worthwhile since apart from this omission the C library builds and runs as expected.

[aykevl]

• VectorTypeKind: this is similar to an array, and needs the element type checked too
• MetadataTypeKind: this shouldn't end up here either way, if it is needed it should be investigated why it is needed.

[hectorchu]

Why wouldn't metadata end up in the IR if the compiler decides to put it there. Can you explain further why you think "this shouldn't end up here"?

[hectorchu]

To reiterate: this IR that has metadata and vectors comes from compiling .c code. So yes .go code wouldn't have metadata, but why wouldn't .c code not have it?

[hectorchu]

Maybe when compiling .c code the compiler can be instructed not to generate IR metadata? But I don't know anything about that.

[hectorchu]

We're on a roll today! So this panic doesn't show up when compiling for xtensa. I did observe it when compiling for darwin-arm64. But since my main target is xtensa and not macOS, I can live with not merging this change. Closing.

[aykevl]

Why wouldn't metadata end up in the IR if the compiler decides to put it there. Can you explain further why you think "this shouldn't end up here"?

Metadata is normally part of debug intrinsics and such. If you don't know why it's there, you can dump the IR, reduce the C code to a minimal reproducer, etc. Make sure you understand why something is the way it is before making a change, don't just randomly change things until they pass your specific test.

[hectorchu]

I am not just changing things randomly - I just thought it was an oversight of the code. I still think it's allowed that the compiler puts debug intrinsics into the IR. I don't want to change the C code because that is a package used all over the place. But anyway I won't spent further time trying to understand why the metadata is being generated.

[aykevl]

You can reproduce a bug and create a small reproducer without needing to make an upstream change to the library. All I'm asking is to figure out why it happens, what sort of C code results in this.

[hectorchu]

I discovered the C code was using the restrict keyword, if I disable it the error goes away. restrict or __restrict__ allows the compiler to aggressively optimize and vectorize code.