bmx-ng / bcc

A next-generation bcc parser for BlitzMax
zlib License
33 stars 13 forks source link

"wrapper functions for null function error function pointers" commit leads to segfaults #662

Open GWRon opened 2 months ago

GWRon commented 2 months ago

Using bcc with the commit 600b543fd59514d7f170b2148e65286b9dc22bd4 leads to a segfault in my game and test applications (using parts of my game code): image

Using the bcc revision of before that commit and everything works as expected.

The method it calls there has an overloaded variant, dunno if that can lead to it (need to investigate a bit tomorrow) edit - nope, renamed the overloaded method and it still failed.

User hotcakes at our discord also mentioned something about the new bcc (weekly build):

this weeks build wasn't tagged by MSE but MaxIDE just doesn't boot for me :/ (including after a recompile (x86 release)) it runs and quits immediately I see bcc was changed, copied the old version from 143.3.57.20240421 across and tried to recompile all modules and MaxIDE, works fine. Compiling with the new decl.bmx works fine of course, but whatever the new stuff in ctranslator is is using something that I'm guessing relies on a Windows 8 call probably for no good reason, so that's a shame.

It might be related to it.

GWRon commented 2 months ago

Seems the debugger was not able to "look into" the function so I dropped the content of that method call into that function. Now it fails there: image

This led me to the original source of the issue.

This is a short sample code leading to the error:

SuperStrict
Framework Brl.StandardIO

Type TTest
    Field valueModFunc:Double(value:Double)

    Method GetValue:Double(value:Double)
        if not valueModFunc
            return DefaultModFunc(value)
        else
            return valueModFunc(value)
        endif
    End Method

    Function DefaultModFunc:Double(value:Double)
        return 10 * value
    End Function
End Type

Local t:TTest= New TTest

print t.GetValue(123)

So it fails to identify a non assigned function pointer in a field ?

GWRon commented 2 months ago

The issue is still immanent. People experience issues (with the weekly builds) with MaxIDE and bcc for some weeks now.

I think we should at least kinda "discuss" how to approach the issue (if required) or maybe @woollybah could write if he is already investigating (simply assign the issue to you @woollybah ).

As I provided a short "failing" example it should be at least possible to replicate it on your own computers.

HurryStarfish commented 2 months ago

I am a bit confused as to how https://github.com/bmx-ng/bcc/commit/600b543fd59514d7f170b2148e65286b9dc22bd4 is meant to work. I might be misunderstanding something here but as far as I can tell, it introduces wrapper functions for the Null function, resulting on Null being a different value for every function type. But doing this without huge breaking changes would require a whole lot of adjustments to the compiler which I'm not seeing in that commit. Apart from = Null checks and boolean conversions (which seems to be the problem @GWRon is experiencing above), this affects every conversion between different function types as well as conversions between function types and pointer types.

Local f1:Int()
Local f2(s:String)
Local p1: Byte Ptr = f1
Local p2: Byte Ptr = f2
Print p1 = p2

still needs to work. So do similar comparisons in Extern functions whenever BlitzMax functions are passed to them, e.g.

Extern "C"
    Function Compare:Int(f1:Int(), f2(s:String))
    Function IsNullFunc:Int(f:Byte Ptr)
End Extern

Local f1:Int()
Local f2(s:String)
Print Compare(f1, f2)
Print IsNullFunc(f1)
Print IsNullFunc(f2)
#include <brl.mod/blitz.mod/blitz.h>

BBINT Compare(void (*f1)(), void (*f2)()) {
    return f1 == f2;
}

BBINT IsNullFunc(void (*f)()) {
    return f == &brl_blitz_NullFunctionError;
}

which has been working since all the way back in legacy BlitzMax.

Isn't there be an easier way to fix the casting issues in https://github.com/bmx-ng/bcc/issues/659 that doesn't require all this?

HurryStarfish commented 2 months ago

On a related note, while writing the above I noticed that function type variance is currently broken. That is an unrelated bug, but such conversions would be relevant here as well (If f = f_ ...).