What happened to the Emscripten branch?

[angros47]

I don't doubt, and it can be a temporary solution, but the new version uses a different syntax that, due to a bug in Emscripten, is not accepted, while it's accepted in GCC. So, one solution is to modify FreeBasic, to accomodate the bug in Emscritpen, the other solution is to fix Emscripten's bug.

[SARG]

I worked a bit on this issue.
Vanya reported that it works with version 1.08 but not with the next ones.
In fact he used an old work from Jeff's repository so not the official 1.08 version.
Vanya provided me this unofficial version and I found that the difference is due to a change in ir-hlc.bas line 2172++ : for arrays a cast is done with integer instead ubyte.
So maybe it's possible to fix the issue by adding a test in case of generation for emscripten to keep the ubyte cast

[SARG]

I tweaked a bit ir-hlc.bas and Vanya tested the change on his program without getting any problem.
For those interested change in ir-hlc.bas, line 2711 with

Code: Select all

if( ( is_c_array ) and ( fbGetOption( FB_COMPOPT_TARGET ) <> FB_COMPTARGET_JS ) ) then

[angros47]

Good point. I don't know why the code has been changed, perhaps CoderJeff knows.

By the way, I still think this should be only a temporary solution: the issue is caused by a bug in Emscripten, and we don't know what other issues such a bug could cause, so it would be better to fix it (and once fixed, with some luck this change should not be necessary anymore)

[SARG]

I totally agree, just a temporary workaround.

The comment in source code explaining the change :

'' Cast to intptr_t to work around gcc out side of array bounds
'' warnings if we are casting from FBSTRING array to pointer
'' fbc uses a kind of virtual pointer for the an array's (0,..)
'' index; technically this is undefinded behaviour in C and is
'' impossible to cast away even when using pointer only casts
'' in the same expression. Some gcc optimizations cause a
'' a warning when setting a pointer for the array's virtual
'' index location. To fix this for compliant C code, would
'' need to rewrite the array descriptor to contain only the
'' offset value from actual memory pointer and compute the
'' array access fully on each array element access.

[coderJeff]

I don't know why the code has been changed, perhaps CoderJeff knows.

The change was intended to fix this bug: https://github.com/freebasic/fbc/issues/217

Problem with fbc's array indexing design is that it uses a calculated index_ptr of where array(0, 0, 0, ... ) would be in memory even if array(0, 0, 0 ... ) is outside the bounds of the array's subscripts. This can result in an index_ptr that is outside the actual range of memory for the actual array elements. Pointer indexing math on pointers outside the range of the array is undefined behaviour in C.

Some C compilers seem to pick up on our cavalier use of pointer math in various warnings, and many don't.

[angros47]

That's good to know. So, the patch wouldn't break anything, I was concerned because I thought that patch was related to all the changes you made to make FreeBasic able to work with C++ ABI

Anyway, if Emscripten will fix that bug (they seem to admit there is a bug in the WASM emitter) this patch could be unnecessary, so I'd wait before adding it to the main branch

Also, do you think a subforum about Emscripten platform would be useful?

[coderJeff]

Also, do you think a subforum about Emscripten platform would be useful?

ok, added in platforms. I grabbed most of the emscripten related topics. If there are others, the moderators should be able to help move them around.

[angros47]

That's great!

[coderJeff]

I tweaked a bit ir-hlc.bas and Vanya tested the change on his program without getting any problem.
For those interested change in ir-hlc.bas, line 2711 with

Code: Select all

if( ( is_c_array ) and ( fbGetOption( FB_COMPOPT_TARGET ) <> FB_COMPTARGET_JS ) ) then

I'm going to go ahead and add this in. One less failure in the test-suite when added in. Should have it committed by today.
Probably not the last time will have to visit -gen gcc emitter as gcc gets stricter in newer versions / standards.

[angros47]

I'm going to go ahead and add this in. One less failure in the test-suite when added in. Should have it committed by today.
Probably not the last time will have to visit -gen gcc emitter as gcc gets stricter in newer versions / standards.

Then, perhaps will you check also viewtopic.php?t=32388? It causes no harms, but it's technically an undefined behavior, and I guess it could be fixed by having the emitter adding some parenthesis

[angros47]

Tried the last version of Emscripten, they haven't fixed that bug yet