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

cmd/6c: miscompiles valid C code. #8454

Closed
OneOfOne opened this issue Jul 31, 2014 · 5 comments
Closed

cmd/6c: miscompiles valid C code. #8454

OneOfOne opened this issue Jul 31, 2014 · 5 comments

Comments

@OneOfOne
Copy link
Contributor

What does 'go version' print?
go version devel +ec4b62f06a49 Wed Jul 30 17:08:33 2014 -0700 linux/amd64
(and) go version go1.3 linux/amd64

What steps reproduce the problem?
1. download both files from https://gist.github.com/OneOfOne/fdab6439ea89871e0769 to
./nocgo
2. run go build && ./nocgo

What happened?
134488224 #wrong hash
981225178

What should have happened instead?
981225178
981225178

Please provide any additional information below.

If you uncomment/comment the mentioned lines in both files and run the function through
cgo, it prints the correct value.

Also a side question, I know there are plans for rewriting everything in Go, however
will the 9p compilers be completely removed or will they remain for C-addons without
using cgo?
@minux
Copy link
Member

minux commented Jul 31, 2014

Comment 1:

the C compiler will eventually go away. Plan for that, so
don't rely on it.
Note the Plan 9 C compiler isn't fully ANSI compliant,
and we're not going to fix bugs in it (because we control
both the compiler and its input, we will just workaround
its bugs).

Status changed to Unfortunate.

@OneOfOne
Copy link
Contributor Author

Comment 2:

http://stackoverflow.com/questions/25048775/am-i-doing-something-wrong-or-is-this-a-bug-in-gos-c-compiler/25049681#25049681
Changing the C function signature to `void ·XXH32_test(const unsigned char* input, U32
len, U32 seed, U32 *ret)` and assigning the hash to *ret seems to return the right value.
I still think this is a bug.

@OneOfOne
Copy link
Contributor Author

Comment 3:

Will the assembler remain?

@minux
Copy link
Member

minux commented Jul 31, 2014

Comment 4:

Yes, 6a will remain. And it will be written in Go too.
The ultimate goal for 6a is that it will be based on latest
intel manuals, so it will support all the latest instructions.
Anyway, for xxHash, I suggest you'd better write it in Go.
The first reason is that the Plan 9 C Compiler will go away
eventually, and 2nd is that 6g will become better over time,
but we definitely won't spend time to improve 6c.

@minux
Copy link
Member

minux commented Aug 9, 2014

Comment 5:

R3 #2,
6c and 6g are using different function call ABI in that
go can't read return value from C functions. This is not
a bug (C doesn't support multiple return values.)
you need to do this:
#define FLUSH(x) USED(x)
void ·XXH32_test(const unsigned char* input, U32 len, U32 seed, U32 ret) {
    // calculate ret
    ret = 42;
    FLUSH(&ret);
}
but still, use pure Go is much better.

@golang golang locked and limited conversation to collaborators Jun 25, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants