search

cznic / virtual

github.com/cznic/virtual has moved to modernc.org/virtual
https://godoc.org/modernc.org/virtual
BSD 3-Clause "New" or "Revised" License
20 stars 3 forks source link

Windows support: strategy question (linux emulation or "native") #1

Closed steffengy closed 7 years ago

steffengy commented 7 years ago

Not sure if this is the right repository to place this issue, since it also affects ccir, but i feel it might belong a bit more here.

Is a windows implementation planned?

I see two very different ways of implementing it:

Which of those (or maybe an other even better idea?) would you consider the favorable solution?

Other todos:

cznic commented 7 years ago

Making those changes (not final) fixes it for fcntl.h:

Great! Please push the changes, I want to cross-check for Linux, thank you.

The only issue I see with this is handling the copyright information since we cannot just insert them at file boundaries can we?

No problem with that.

While writing this I actually have a potential solution of inserting them at the top of the file and referencing them at bounds which might be best illustrated with an example:

[COPYRIGHT_1] Copyright Super Cool Corporation ...
[COPYRIGHT_2] Copyright XYZ! DO THAT! DONT DO THAT ....

// BOUNDARY: BELOW HERE [COPYRIGHT_1] TILL NEXT BOUNDARY:
int company_1_test();
int company_1_test2();
// BOUNDARY: BELOW HERE [COPYRIGHT_2] TILL NEXT BOUNDARY:
int company_2_test();
int company_2_test_2();
// BOUNDARY: BELOW HERE [COPYRIGHT_1]
int company_1_test();
// BOUNDARY: BELOW HERE [COPYRIGHT_2]
int company_2_test();
Any idea about that?

No problem either, but sounds like more work.

Reproducing the copyright notices is very important. Having them sorted and/or grouped near the top of the file is nice and somewhat preferable, but not important. Please use any simple solution. We can improve on this later, but we cannot distribute the generated headers without the proper attribution. (I am not a lawyer, but that's what I think.)

steffengy commented 7 years ago

Yeah I'll implement it and push it, doesn't sound like too much work. Maybe give me a couple minutes :)

cznic commented 7 years ago

No hurry. You're already near c ;-)

steffengy commented 7 years ago

@cznic Sorry took a bit longer than expected, should work though, let me know :)

cznic commented 7 years ago

All indicators green on Linux!

steffengy commented 7 years ago

Nice! windows.h also is miles further than before :)

steffengy commented 7 years ago 
panic: windows_windows_amd64.h:476:9: TODO

goroutine 1 [running]:
github.com/cznic/cc.(*StructOrUnionSpecifier).isCompatible(0xc04500bf20, 0xc04500bda0, 0x7c3d8)
        D:/projects/Go/gopath/src/github.com/cznic/cc/ast2.go:3394 +0x577

Is CASE1 a trivial case or something that requires quite a bit of logic? Since CASE0 doesn't seem to check the SpecifierQualifierList, I guess it's trivial?

https://github.com/cznic/cc/blob/master/ast2.go#L3362-L3394

cznic commented 7 years ago

Is CASE1 a trivial case or something that requires quite a bit of logic?

Not sure yet, on it.

cznic commented 7 years ago

Please push the windows_windows_amd64.h file, thanks. I cannot imagine how we can hit case 1. It's probably yet another thing I don't know about C :open_mouth:

cznic commented 7 years ago

Hmm, it must be an anonymous field, right?

steffengy commented 7 years ago

Quite possible. MINGW is full of anonymous unions, structs, etc ^^ Files are regenerated and pushed.

cznic commented 7 years ago

Fix attempt https://github.com/cznic/cc/commit/3c633d52f0d0a5db61b46d58ddb18a879075c339 pushed. Please try it.

steffengy commented 7 years ago 
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xc0000005 code=0x0 addr=0x8 pc=0x539be1]

goroutine 1 [running]:
github.com/cznic/cc.(*SpecifierQualifierList).isCompatible(0xc0481822d0, 0xc04813a720, 0x40b291)
        D:/projects/Go/gopath/src/github.com/cznic/cc/ast2.go:3117 +0x51
github.com/cznic/cc.(*StructOrUnionSpecifier).isCompatible(0xc0485b5a40, 0xc0485b5920, 0x7c3ca)
        D:/projects/Go/gopath/src/github.com/cznic/cc/ast2.go:3409 +0x49d
github.com/cznic/cc.(*Bindings).defineStructTag(0xc044fe35f0, 0x2caf4c0000e021, 0x7c3ca, 0x7434e0, 0xc0485b5a40, 0xc046258660)
cznic commented 7 years ago

Just return true for now (shame on me): https://github.com/cznic/cc/commit/563948c33bbc4e7373939ff23637f66cc3131cd0

steffengy commented 7 years ago

Haha I have a feeling the windows headers are hitting quite some unusual cases:


panic: windows_windows_amd64.h:873:9: TODO

goroutine 1 [running]:
github.com/cznic/cc.(*StructOrUnionSpecifier).isCompatible(0xc043575920, 0xc043575800, 0xa7c0d)
        D:/projects/Go/gopath/src/github.com/cznic/cc/ast2.go:3397 +0x347```
steffengy commented 7 years ago

I'll add a .isCompatible to StructDeclarator and StructDeclaratorList and refactor it...

cznic commented 7 years ago

Collaborator's invite for cc on the way ;-)

cznic commented 7 years ago

Warning: Writing cc, a year or so ago, was for me actually a way to start learning C. The code base is probably the worst Go code I've ever published. Its original purpose was to handle just and only exactly one file, nothing else was originally important. The name of that file was/is sqlite3.c ;-)

cznic commented 7 years ago

I'll add a .isCompatible to StructDeclarator and StructDeclaratorList and refactor it...

Appreciated, thank you!

steffengy commented 7 years ago

Done: https://github.com/cznic/cc/commit/01bc451bc7f1cba70812acfcc275f8c4395841ac

could you take a look at this since the following comment IMO was wrong and was replaced with the correct one: // StructDeclaratorList ',' StructDeclarator // Case 1 --> // DeclaratorOpt ':' ConstantExpression // Case 1

The tests don't seem to show any change to master.

cznic commented 7 years ago

could you take a look at this since the following comment IMO was wrong and was replaced with the correct one:

I found one comment which is probably incorrect, but I'm not sure where the change you refer to is located?

// StructDeclaratorList ',' StructDeclarator // Case 1

Please link to the diff line, thank you.

steffengy commented 7 years ago

https://github.com/cznic/cc/commit/01bc451bc7f1cba70812acfcc275f8c4395841ac#diff-69cc9cc7ea094f7c11d142b0a77d423cL3396

Probably the same origin, that's the one I corrected since it was bogus IMO.

cznic commented 7 years ago

Yeah, that was wrong AFAICT. Good catch, I've got confused by that today already.

steffengy commented 7 years ago

Would it be difficult to support anonymous unions as struct members in cc?

typedef struct _OVERLAPPED {
  ULONG_PTR Internal;
  ULONG_PTR InternalHigh;
  union {
    struct {
      DWORD Offset;
      DWORD OffsetHigh;
    };
    PVOID  Pointer;
  };
  HANDLE    hEvent;
} OVERLAPPED, *LPOVERLAPPED;

(from MSDN directly but identical to MINGW definition)

SQLite accesses e.g. overlapped.Offset

cznic commented 7 years ago

Should be possible via EnableAnonymousStructFields.

If that option is already used, then we have another problem to solve.

cznic commented 7 years ago

Maybe the problem is the selector per se, not able to find the field...? (TODO in cc perhaps)

steffengy commented 7 years ago

EnableAnonymousStructFields is already specified for sqlite. Any idea where I'd have to look to find that selector & relevant stuff?

steffengy commented 7 years ago

Found it: https://github.com/cznic/cc/blob/master/etc.go#L976

steffengy commented 7 years ago

By accident I also noticed this is actually wrong in the generated header:

typedef struct _OVERLAPPED {ULONG_PTR Internal ;ULONG_PTR InternalHigh ;union {struct {DWORD Offset ;DWORD OffsetHigh ;}s ;PVOID Pointer ;}u ;HANDLE hEvent ;}OVERLAPPED ;

seems like some IFDEF is missing... (the union is named "u")

cznic commented 7 years ago

Found it: https://github.com/cznic/cc/blob/master/etc.go#L976

Yep. It's a flat search and it should be a BFS tree one actually. I think C99 prescribes the same resolution algorithm as Go: The shortest unambiguous path wins.

steffengy commented 7 years ago

Adding _MSC_EXTENSIONS fixed the wrong generated code, problem persists.

I'll probably just implement a recursive search into all unnamed members and throw an error when there are more than one results, does that sound okay?

cznic commented 7 years ago

'll probably just implement a recursive search into all unnamed members and throw an error when there are more than one results, does that sound okay?

It does as that probably covers 99% of cases.

cznic commented 7 years ago

However, I'm afraid ccir probably does not know yet how to handle foo.bar for bar being a member of anonymous struct/union field of foo.

steffengy commented 7 years ago

I pushed a commit, I'll see if it causes any issues... I hope it's enough

cznic commented 7 years ago

Finding the member is one part of the problem. I guess the member's .Offset is not okay. It's correct within its parent anonymous struct but not necessarily correct within its non-anonymous super-parent struct if the anonymous parent has more than one field and the field found is not the first one. (Which it often is.)

Just saying that I have a feeling there's unfortunately more to this that has to be done, probably both in cc and ccir.

cznic commented 7 years ago

I pushed a commit, I'll see if it causes any issues... I hope it's enough

Thanks a lot.

cznic commented 7 years ago

FYI:

jnml@4670:~/src/github.com/cznic/cc$ git pull
remote: Counting objects: 6, done.
remote: Compressing objects: 100% (6/6), done.
remote: Total 6 (delta 1), reused 0 (delta 0), pack-reused 0
Unpacking objects: 100% (6/6), done.
From github.com:cznic/cc
 + 01bc451...520afcb win_support -> origin/win_support  (forced update)
Auto-merging ast2.go
CONFLICT (content): Merge conflict in ast2.go
Automatic merge failed; fix conflicts and then commit the result.
jnml@4670:~/src/github.com/cznic/cc$
steffengy commented 7 years ago

yeah i fixed that wrong comment, so you'll probably have to resolve that conflict (or reset to a commit before & pull)

cznic commented 7 years ago

yeah i fixed that typo, so you'll probably have to resolve that wrong comment (or reset to a commit before & pull)

NP, thanks again.

Morning alarm in ~6 hours, have to force myself to bed now. I'll try to watch for any Github notifications, but I can process them most likely not before tomorrow evening.

steffengy commented 7 years ago

Yeah same for me...

%v ..\..\..\sqlite.org\sqlite-amalgamation-3180000\sqlite3.c:40170:5: internal error: struct _OVERLAPPED

seems anyway like I won't get that done too fast...

https://github.com/cznic/ccir/blob/c0e583fb1a239a34c2ddbaa35837f36c5d60de94/ccir.go#L1206

steffengy commented 7 years ago

I solved the anonymous union feature in my cznic/cc branch by transforming a.b to *(ty*)((&a)+offset) on the AST level. (where offset is the entire offset of b) This only happens when it doesn't find a direct member and it can find such a field using a recursive search. Hopefully the offset calculation is correct, but afaik it should. The transformation happens on the AST (during constant-evaluation afaik). It was the simplest way I could think of for implementation.

SQLITE now builds and links. So basically only the OPCodes are missing now (stubs are added in the cznic/virtual branch).

Now finally getting the 2.5 hours of sleep I've left till work :)

cznic commented 7 years ago

I solved the anonymous union feature in my cznic/cc branch by transforming a.b to (ty)((&a)+offset) on the AST level. (where offset is the entire offset of b) This only happens when it doesn't find a direct member and it can find such a field using a recursive search. Hopefully the offset calculation is correct, but afaik it should.

Nice, looking forward testing it.

SQLITE now builds and links. So basically only the OPCodes are missing now (stubs are added in the cznic/virtual branch).

Great! So you should already get a VM stacktrace pointing to the first unimplemented builtin encountered. Also, running go generate in cznic/sqlite should now work.

Now finally getting the 2.5 hours of sleep I've left till work :)

Ouch ;-)

steffengy commented 7 years ago

Adding EnableWideBitFieldTypes() to sqlite's generator.go allows go generate to work, yes.

generator.go:365: code 0x0003c979, text 0x00006c90, data 0x00006af0, bss 0x000004b0, pc2func 1930, pc2line 50253, symbols 302, gz 627417

Filesize of bin_windows_amd64.go is 4 bytes bigger than linux, so reasonable.

Some questions:

cznic commented 7 years ago

Adding EnableWideBitFieldTypes() to sqlite's generator.go allows go generate to work, yes.

generator.go:365: code 0x0003c979, text 0x00006c90, data 0x00006af0, bss 0x000004b0, pc2func 1930, pc2line 50253, symbols 302, gz 627417

Filesize of bin_windows_amd64.go is 4 bytes bigger than linux, so reasonable.

Very nice!

AFAIK virtual doesn't support multithreading yet right? (there doesn't seem to be a possibility to create an actual new thread, atleast pthread_create doesn't exist)

It does not because sqlite has never tried to call this builtin so it's not yet implemented. However, every sql.DB connection runs in it's own VM thread.

I just noticed because of an SQLite assertion that CCIR does define long with size==8, while on windows size==4 is expected. I changed that in ccir/model.go but I'm getting some strange error from cznic/ir:

That will be a problem. The assumption of sizeof(long) == 4 on 32 bit archs and 8 on 64 bit archs is used in many places. I cannot find the SQLite assertion (assert.*sizeof.*long), please tell me the line number.

Verify (A): mismatched types, got int32, expected int64

The verifier is simple but just proved effective ;-)

any idea what else should be done to change the size?

I want to first check and understand why SQLite insists on 32 bit long. On second thought, it may be enough to specialize virtual/cpu64.go, ie. split it into cpu64_linux.go and cpu64_windows.go. It's the place where long ints are handled in an architecture specific way but with a unified "api".

steffengy commented 7 years ago

I cannot find the SQLite assertion (assert.sizeof.long), please tell me the line number.

It isn't SQLite directly, rather Windows/Microsoft/MINGW let me explain: The actual assertion that fails is assert( winSysInfo.dwAllocationGranularity>0 ); (L43634)

winSysInfo is of type SYSTEMINFO and is passed to GetSystemInfo Then GetSystemInfo (WINAPI) populates this struct with information, assuming standard windows types.

Since the struct contains LONG fields (DWORD resolves to long) we can't get parity. We have a type mismatch, WinAPI writes to other offsets than we'd read from. So we only get total non-sense back (In this case the assertion fails with getting 0, luckily).

In summary, all windows code will assume SIZEOF(LONG)==4 on every architecture directly or indirectly one way or another.

cznic commented 7 years ago

In summary, all windows code will assume SIZEOF(LONG)==4 on every platform directly or indirectly one way or another.

Okay, the virtual/cpu64.go split is IMO the starting point. Let's see if I have properly routed every long access through the {read,write}[U]long routines. Near the top is a longbits constant, need to be changed for Windows as well.

steffengy commented 7 years ago

Okay, but isn't IR running before VM? So here we have a codegen problem?

sqlite3.c:22340:7 all_test.go:823: # [287]: Verify (A): mismatched types, got int32, expected int64

https://github.com/cznic/ir/blob/master/operation.go#L522

steffengy commented 7 years ago

Did the split between cpu64.go and cpu64_windows.go c699e43ee86a9c472a26672f762e5140dd54da91.

The IR error still persists, still wondering if it's related to wrong codegen (seems like a uint32 = uint64 convert) or something else.

all_test.go:823: # [287]: Verify (A): mismatched types, got2 int32, expected int64
 memsys5Size:0x2e:       convert                 int64, int32    ; sqlite3.c:22340:7
  # [287, err]: *ir.FunctionDefinition {sqlite3.c:22337:1 memsys5Size func(*struct{})int32 InternalLinkage } [p]

Any idea?

PS

just running go test -run TestSqlite3 at the moment since GCC seem to fail indeed in the vm gcc.c-torture\execute\20041214-1.c

unexpected fault address 0x102aaff68
fatal error: fault
[signal 0xc0000005 code=0x0 addr=0x102aaff68 pc=0x648252]

goroutine 316 [running]:
runtime.throw(0x75a480, 0x5)
        Go/src/runtime/panic.go:596 +0x9c fp=0xc042a18670 sp=0xc042a18650
runtime.sigpanic()
        Go/src/runtime/signal_windows.go:164 +0xff fp=0xc042a186a0 sp=0xc042a18670
github.com/cznic/virtual.(*cpu).run(0xc04208c700, 0x2, 0x0, 0x0, 0x0)
        cznic/virtual/cpu.go:987 +0x2202 fp=0xc042a190a8 sp=0xc042a186a0
github.com/cznic/virtual.New(0xc0423f4780, 0xc042414dd0, 0x1, 0x1, 0x8ab020, 0xc04230fab0, 0x8ac9a0, 0xc042004b18, 0x8ac9a0, 0xc042004b20, ...)
       cznic/virtual/virtual.go:73 +0x2de fp=0xc042a19168 sp=0xc042a190a8
github.com/cznic/virtual.Exec(0xc0423f4780, 0xc042414dd0, 0x1, 0x1, 0x8ab020, 0xc04230fab0, 0x8ac9a0, 0xc042004b18, 0x8ac9a0, 0xc042004b20, ...)
       cznic/virtual/virtual.go:84 +0xf0 fp=0xc042a19220 sp=0xc042a19168
github.com/cznic/ccir.expect1.func3(0xc0420aca00, 0x2f, 0x7734b8, 0xc04205caf0, 0x49, 0xc042a19b20, 0xc0423f4780, 0xc04230fab0, 0xc042004b18, 0xc042004b20, ...)
        cznic/ccir/all_test.go:287 +0x246 fp=0xc042a19350 sp=0xc042a19220
cznic commented 7 years ago

Okay, but isn't IR running before VM?

No, but the verifier also computes the stack structure of all possible code paths and checks that jump targets are stack-structure compatible with the jump origin stack. This error says the declared IR instruction operand type does not match the computed stack operand. That's bad, the program may easily crash when executed later. The error should be accompanied with an IR dump of the offending function. From it I hope I can find where an invalid leak of the assumption longbits=64 on Windows is. (Assuming you've already split cpu64.go)

If it does not produce the dump, maybe running go run generator.go -log will produce it. Otherwise I'll add the option. Please put the dump anywhere for me to look at, thank you.

I hope to be at home in about one hour, will try to resolve this. It's a pitty I never contemplated checking int sizes on Windows. Or that they are more restricted than C99 prescribes. TIL

So here we have a codegen problem?

Yes, ccir produced invalid IR. It's a bug.

cznic commented 7 years ago

just running go test -run TestSqlite3 at the moment since GCC(?) tests etc. seem to fail indeed in the vm

My lesson learned was to not skip tests in ccir. They are in order of complexity (TCC, GCCExec, Sqlite) for this reason.