Recent changes to tds.go mean compilation on GOARCH=386 is no longer possible

[jimbobmcgee]

Describe the bug
Recent changes to tds.go mean compilation on GOARCH=386 is no longer possible. As far as I can tell, it is only this file currently preventing support for 386.

To Reproduce

set GOARCH=386
go build any_go-mssql_based_prog.go

Expected behavior
Compilation should be able to succeed, regardless of GOARCH

Further technical details

Line 489 (as of v0.15) reads...

nlen8 := len(s) & 0xFFFFFFF8

The len(s) infers int, which is 32 bits on x86 platforms, but 0xFFFFFFF8 does not coerce to a 32-bit int. I can change this to var nlen8 int64 = int64(len(s)) & 0xFFFFFFF8 and add some extra casts further down to resolve simple comparison mismatches, and it will compile, but there is some unsafe.Pointer math going on in/around this variable, which I don't immediately understand, and I am not confident it won't lead to a problem further down the line if I PR based on this simplistic fix.

Additional context
In the meantime, I have had to vendor the module, and cherry-pick func ucs22str from the previous version. If the pointer math is not trivial to resolve in x86, perhaps the sanest approach is to move this function out of tds.go into its own code files ucs22str.go and ucs22str_386.go and use +build tags to conditionally compile both versions -- I could PR that if it suits?

[jimbobmcgee]

(pinging @PeteBassettBet365, as author of affected func ucs22str...)

[shueybubbles]

@jimbobmcgee it would be fine to add a 386-specific .go file to fix this. Additionally, since 32 bit compilation is still needed, can you add a 32bit build to the appveyor PR verification job? I don't think it's being tested at all.

[shueybubbles]

Would there be any value in having the 32bit version of the code revert to the prior non-optimized behavior for now? it might be a quicker solution to implement

[jimbobmcgee]

@shueybubbles...

Would there be any value in having the 32bit version of the code revert to the prior non-optimized behavior for now? it might be a quicker solution to implement

Yes, that's indeed what I meant, when I suggested using the build tags...

• create two new files -- ucs22str_opt.go and ucs22str_unopt.go
• move the current, optimised func ucs22str definition to ucs22str_opt.go
• copy the old non-optimised func ucs22str definition to ucs22str_unopt.go
• tag ucs22str_opt.go with // +build !386
• tag ucs22str_unopt.go with // +build 386

It may then be feasible to do x86-based pointer maths in a later iteration (for someone who understands what that unsafe.Pointer code is doing in the optimised version).

It is worth noting that it might be that the optimised version only works on x64, in which case +build amd64 and +build !amd64 would be the better choice.

Of course, if that someone thinks the pointer maths is easy enough to port to all supported platforms, then doing so in a single ucs22str.go file within a switch GOARCH { } might be cleaner for everyone (and might be a better long-term goal).

Additionally, since 32 bit compilation is still needed, can you add a 32bit build to the appveyor PR verification job? I don't think it's being tested at all.

I am not familiar with Appveyor. Is it something you can add to the PR (if you'd like me to raise one)?

[shueybubbles]

if you fix the 32bit compile as you described with the second go file, I will add the 32bit build to the PR checks myself.
It will require updating the matrix in https://github.com/microsoft/go-mssqldb/blob/main/appveyor.yml

[jimbobmcgee]

Having just seen your CONTRIBUTING.md, I might not be able to make this change for you. I will check on Monday.

[jimbobmcgee]

@shueybubbles - as feared; work are happy enough for me to submit the patch, but not happy enough to countersign a CLA document.

Please advise how/if you want me to proceed.

[shueybubbles]

Ok we'll try to get to it soon.
AFAIK our CLA is just asking you to confirm you aren't pirating someone else's code, but I'm not a lawyer so I won't claim true expertise.

[shueybubbles]

Testing with GOARCH=386 exposes some other bugs

--- FAIL: TestDateTimeParam19 (0.84s)
    --- FAIL: TestDateTimeParam19/Test_datetime_for_{997000000_315537897599_<nil>} (0.06s)
        queries_go19_test.go:1045: expected: '9999-12-31 23:59:59.997 +0000 UTC', got: '9999-12-31 23:59:58.997 +0000 UTC' delta: 1000000000
2022/07/26 09:48:36 [{Hello} {World} {TVP}]