rochus-keller / Oberon

Oberon parser, code model & browser, compiler and IDE with debugger, and an implementation of the Oberon+ programming language
GNU General Public License v2.0
463 stars 30 forks source link

Handling strings from external sources #22

Closed tenko closed 2 years ago

tenko commented 2 years ago

When experimenting with a wrapper of Sqlite I was wondering if there was a cleaner way to handle strings returned from functions in the wrapped dll.

Wrapper:

definition Sqlite [extern 'C', dll 'libsqlite3-0']
    type
        DB = cstruct end

    const
        OK = 0      // Successful result
        ERROR = 1   // Generic error

    proc libversion_number(): integer [alias 'sqlite3_libversion_number']
    proc libversion(): *[]char [alias 'sqlite3_libversion']
    proc open(filename: *[]char; dbref: *[]*DB): integer [alias 'sqlite3_open']
    proc close(db: *DB) [alias 'sqlite3_close']
    proc errmsg(db: *DB): *[]char [alias 'sqlite3_errmsg']  
end Sqlite

Test code:

module Test

import Sqlite

proc errmsg(msg : *[]char)
var
    buf : array 256 of char
    i : integer
begin
    while (msg[i] # 0x) & (i < 256) do
        buf[i] := msg[i]
        inc(i)
    end 
    buf[i] := 0x
    println(buf)    
end errmsg

var
    db: carray 1 of *Sqlite.DB
begin
    println(Sqlite.libversion_number())
    errmsg(Sqlite.libversion())

    if  Sqlite.open("test.db", db) # Sqlite.OK then
        errmsg(Sqlite.errmsg(db[0]))
        Sqlite.close(db[0])
        halt(1)
    end 
    Sqlite.close(db[0])
    println("Done")
end Test

In order to print the error message a separate procedure print was introduced. Obviously the compiler does not know if the returned array of char is expected to be zero terminated, but I expected an array copy buf := msg to work. Is there something I am missing here?

rochus-keller commented 2 years ago

You can directly assign buf := msg in errmsg(), so the while loop is redundant. According to the sqlite docu the returned string is zero terminated and that's what the compiler expects from both array of char as well as carray of char. println() currently prints the address in case of cpointer; but you can explicitly dereference it to print the string directly, so the copy is not necessary: println(msg^)

tenko commented 2 years ago

Very good. Probably some problems in the C generator then.

With the buf := msg i get the following:

gcc -O0 --std=c99 -static build/*.c -o build/Test.exe -lm -DOBX_USE_BOEHM_GC -lgc
build/Test.c: In function 'Test$errmsg':
build/Test.c:9:52: warning: passing argument 3 of 'OBX$StrCopy' from incompatible pointer type [-Wincompatible-pointer-types]
    9 |     OBX$StrCopy(&(struct OBX$Array$1){256,1,buf},0,msg,0);
      |                                                    ^~~
      |                                                    |
      |                                                    char *
In file included from build/Test.h:6,
                 from build/Test.c:3:
build/OBX.Runtime.h:68:87: note: expected 'const struct OBX$Array$1 *' but argument is of type 'char *'
   68 | extern void OBX$StrCopy(struct OBX$Array$1* lhs, int lwide, const struct OBX$Array$1* rhs, int rwide );
      |                                                             ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~

When I run the resulting .exe I get an segmentation fault.

With

proc errmsg(msg : *[]char)
begin
    println(msg^)
end errmsg

I get the following error:

gcc -O0 --std=c99 -static build/*.c -o build/Test.exe -lm -DOBX_USE_BOEHM_GC -lgc
build/Test.c: In function 'Test$errmsg':
build/Test.c:7:36: error: request for member '$a' in something not a structure or union
    7 |     OBX$PrintA(1,(const char*)(msg).$a);
      |                                    ^
make: *** [Makefile:14: build/Test.exe] Error 1
rochus-keller commented 2 years ago

Oops, then I have something to do today.

rochus-keller commented 2 years ago

I had to refactor the C code generator and to add some clarifications to the language specification; arrays and carrays are now handled the same way in the generated code which makes it easier to assign between the two; the new version is committed to github; do you need a pre-compiled version?

Here is the modified section in the language spec:

Type interoperability

ARRAY and RECORD types cannot be used in external library modules, but it is perfectly legal to use C_Types as formal parameter, or local or module variable types in regular Oberon+ modules. CPOINTER (but not structured C_Types) can be used as field or element type in RECORD or ARRAY. Structured C_Types (in contrast to CPOINTER to structured C_Types) cannot be used as formal VAR or IN parameters.

POINTER and CPOINTER are disjoint in what they can point to and it is not possible to assign from a POINTER to a CPOINTER or vice versa.

A CARRAY and an ARRAY are only assignment compatible if both element types are either CHAR or WCHAR. A CARRAY cannot be passed to a parameter of ARRAY type.

tenko commented 2 years ago

I built it myself and it now works, but got an error related to linking which was resolved with gcc flag -fcommon

gcc -O0 --std=c99 -static build/*.c -o build/Test.exe -lm -DOBX_USE_BOEHM_GC -lgc
C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.1.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:\msys64\tmp\ccGVSw5u.o:OBX.Runtime.c:(.bss+0x0): multiple definition of `OBX$Anyrec$class$'; C:\msys64\tmp\cciJjefB.o:OBX.Main.c:(.bss+0x0): first defined here
C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.1.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:\msys64\tmp\ccGVSw5u.o:OBX.Runtime.c:(.data+0x0): multiple definition of `OBX$defaultException'; C:\msys64\tmp\cciJjefB.o:OBX.Main.c:(.bss+0x8): first defined here
C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.1.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:\msys64\tmp\ccwO894v.o:Sqlite.c:(.bss+0x0): multiple definition of `OBX$Anyrec$class$'; C:\msys64\tmp\cciJjefB.o:OBX.Main.c:(.bss+0x0): first defined here
C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.1.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:\msys64\tmp\ccwO894v.o:Sqlite.c:(.bss+0x8): multiple definition of `OBX$defaultException'; C:\msys64\tmp\cciJjefB.o:OBX.Main.c:(.bss+0x8): first defined here
C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.1.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:\msys64\tmp\ccIU1mZz.o:Test.c:(.bss+0x0): multiple definition of `OBX$Anyrec$class$'; C:\msys64\tmp\cciJjefB.o:OBX.Main.c:(.bss+0x0): first defined here
C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.1.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:\msys64\tmp\ccIU1mZz.o:Test.c:(.bss+0x8): multiple definition of `OBX$defaultException'; C:\msys64\tmp\cciJjefB.o:OBX.Main.c:(.bss+0x8): first defined here
collect2.exe: error: ld returned 1 exit status
make: *** [Makefile:14: build/Test.exe] Error 1

Probably missing some header guards?

rochus-keller commented 2 years ago

There seems to be an issue in mingw 12.x; also another user had the same issue; I was not able to reproduce it with the official mingw windows edition (which is gcc version 8); also the most recent clang doesn't have this issue; I don't know the reason, but it doesn't happen if you either use clang or an older version of gcc.

rochus-keller commented 2 years ago

Ok, I have a suspicion what the issue could be. According to the ISO C standard external linkage is default if nothing is specified. GCC 12.x apparently does it differently. Can you try to prefix lines 51 and 55 of OBX.Runtime.h with "extern"?

tenko commented 2 years ago

With the above changes I get only warnings

gcc -O0 --std=c99 -static build/*.c -o build/Test.exe -lm -DOBX_USE_BOEHM_GC -lgc
In file included from build/Test.h:6,
                 from build/OBX.Main.c:4:
build/OBX.Runtime.h:54:1: warning: useless storage class specifier in empty declaration
   54 | };
      | ^
In file included from build/OBX.Runtime.c:24:
build/OBX.Runtime.h:54:1: warning: useless storage class specifier in empty declaration
   54 | };
      | ^
In file included from build/Sqlite.h:6,
                 from build/Sqlite.c:3:
build/OBX.Runtime.h:54:1: warning: useless storage class specifier in empty declaration
   54 | };
      | ^
In file included from build/Test.h:6,
                 from build/Test.c:3:
build/OBX.Runtime.h:54:1: warning: useless storage class specifier in empty declaration
   54 | };
      | ^

Otherwise it builds and run fine.

rochus-keller commented 2 years ago

The warnings are strange. Does your OBX.Runtime.h look like this:

struct OBX$Anyrec$Class$ {
    struct OBX$Anyrec$Class$* super$;
};
extern struct OBX$Anyrec$Class$ OBX$Anyrec$class$;
struct OBX$Anyrec {
    struct OBX$Anyrec$Class$* class$;
};
extern struct OBX$Anyrec OBX$defaultException;

The optional extern specifiers should only be in front of the two variables with file scope.

I also see that you compile with -static; what's the intention?

tenko commented 2 years ago

You are right. With extern as shown above the warning goes away. The -static was leftover as I intended to look into how to link with a static library.