Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dim shared byref initializer check #87

Merged
merged 9 commits into from Jun 19, 2018

Conversation

Projects
None yet
2 participants
@dkl
Copy link
Collaborator

commented Jun 18, 2018

global ref initializers were not checked for being non-constant, but that can actually matter. For example when trying to initialize a ref from another ref, the second ref will try to copy the first ref's value, which is a non-constant initializer that isn't possible without generating a global constructor.

So for now I think the best solution is to disallow the case - at least fbc doesn't crash anymore.

dkl added some commits Jun 18, 2018

Add test case for DIM BYREF bug #822 (fixed by a821ef5/3cc22e22)
git bisect showed that a821ef5 already fixed this indirectly,
by showing the following error message:
  Local symbols can't be referenced in 'Dim Byref As UDT1 UDT2.ru1 = u1'
now since 3cc22e2 another error is shown:
  Expected constant in 'Dim Byref As UDT1 UDT2.ru1 = u1'

@dkl dkl force-pushed the dkl:byref branch from 9a3cf9c to 392feb4 Jun 18, 2018

@jayrm

This comment has been minimized.

Copy link
Collaborator

commented Jun 18, 2018

Looks good to deal with the const initializer checks.

My only recommendation is, that after this PR is merged, is to close 814, 822, 842, and start a new ticket for the following:

bug ticket 822 has this example:

Type UDT1
    Dim As Integer I1
End Type

Type UDT2
    Dim As Integer I2
    Static Byref As UDT1 ru1
End Type

Dim Byref As UDT1 UDT2.ru1 = *Cast(UDT1 Ptr, 0)

So we can still arrive at a failed assertion assert( expr <> NULL ) hFlushExprStatic(), but I think this is a separate issue from the initial 822 bug report, and it would be easier to track as separate issue.

@jayrm

jayrm approved these changes Jun 18, 2018

dkl added some commits Jun 18, 2018

Fix emitting of CONST ref initializers (as opposed to OFFSETs)
The CONST code path in hFlushExprStatic() used the symbol type as-is,
instead of converting to pointer type for references. Thus the type
looked different and it attempted the astNewCONV() which could easily
fail (because the init expression is a pointer, while the symbol could
be anything, e.g. struct/string).
@dkl

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 18, 2018

Ok, that one is also interesting. Turns out the CONST code path (as opposed to the OFFSET code path) in hFlushExprStatic() didn't consider that a ref actually is a pointer and tried to do astNewCONV() from the type-casted null pointer initree (pointer type) to the symbol type (struct). The new commit should fix that.

@jayrm

This comment has been minimized.

Copy link
Collaborator

commented Jun 18, 2018

You make it look so easy. I struggled with this for days. You are at about the spot I got stuck trying to convert the LHS sym to just a pointer. x86 emitter doesn't know what to do with FB_DATATYPE_STRUCT in EMITVARINII.

@jayrm

This comment has been minimized.

Copy link
Collaborator

commented Jun 18, 2018

In ten years I have never seen ".invalid" come up in x86 -gen gas. Could use dtype = FB_DATATYPE_STRUCT to mean reference to struct. i.e. only time would ever pass STRUCT is that it is a ref to STRUCT. Maybe.

In emit_x86.bas:EMITVARINII()

	case FB_DATATYPE_STRUCT, FB_DATATYPE_POINTER
		'' reference to struct or pointer
		function = @".long"
	case FB_DATATYPE_STRING
		function = @".INVALID"

Will allow this...

type udt1
	dim as integer i1
end type

type udt2
	dim as integer i2
	static byref as udt1 ru1
end type

dim byref as udt1 udt2.ru1 = *cast(udt1 ptr, 55)

print @udt2.ru1 '' returns 55

to succeed.

Fix hFlushExprStatic() to use full type again (regression from 616dc23)
I can't think of a test case for this though, CONSTness should have been
checked before already, it shouldn't matter here when emitting.
Also the full type is only used for an astNewCONV() which allows casting
away CONSTness etc. anyways.
var sdtype = symbGetType( sym )
var sfulldtype = symbGetType( sym )

This comment has been minimized.

Copy link
@jayrm

jayrm Jun 19, 2018

Collaborator

sfulldtype has same value as sdtype throughout.

This comment has been minimized.

Copy link
@dkl

dkl Jun 19, 2018

Author Collaborator

oops; fixed it.

if( symbIsRef( sym ) ) then
'' Initializers for references initialize the pointer,
'' not an object of the symbol's type.
sdtype = typeAddrOf( sdtype )

This comment has been minimized.

Copy link
@jayrm

jayrm Jun 19, 2018

Collaborator

if this is removed....

@@ -543,7 +551,7 @@ private sub hFlushExprStatic( byval n as ASTNODE ptr, byval basesym as FBSYMBOL
else
'' different types?
if( edtype <> sdtype ) then

This comment has been minimized.

Copy link
@jayrm

jayrm Jun 19, 2018

Collaborator

... then this should be if( edtype <> sfulldtype ) then

@jayrm

This comment has been minimized.

Copy link
Collaborator

commented Jun 19, 2018

There is a lower level issue going on here in either x86 IR or x86 emitter. -gen gcc seems to handle it this change OK, but -gen gas does not. My previous comment is incorrect about replacing ".invalid" for structures in the x86 emitter.

For example, this succeeds for integer but not for byte

Type UDT
	Dim As Integer I
	Static Byref As byte RI
End Type

Dim Byref As byte UDT.RI = *Cast(byte Ptr, 123456)

print @UDT.RI

but will give other ASM errors when emitted.

@dkl dkl force-pushed the dkl:byref branch from 4de3f16 to 5d08b88 Jun 19, 2018

@jayrm

jayrm approved these changes Jun 19, 2018

@dkl dkl merged commit e3fcb8d into freebasic:master Jun 19, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dkl dkl deleted the dkl:byref branch Jun 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.