Skip to content

bytealg.CountString is not supported #424

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

Closed
1l0 opened this issue Jun 27, 2019 · 9 comments
Closed

bytealg.CountString is not supported #424

1l0 opened this issue Jun 27, 2019 · 9 comments
Labels
enhancement New feature or request

Comments

@1l0
Copy link
Contributor

1l0 commented Jun 27, 2019

main.go:

package main

import (
	"fmt"
	"strings"
)

func main() {
	s := strings.Replace("abc", "a", "x", -1)
	fmt.Println(s)
}

run:

$ tinygo run main.go
Undefined symbols for architecture x86_64:
  "_internal/bytealg.CountString", referenced from:
      _main in main.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
error: failed to link /var/folders/kt/gfbj96zj1jvd9tsbbfjcg94h0000gn/T/tinygo175846444/main: exit status 1
$ go run main.go
xbc

How can I fix this?

@aykevl
Copy link
Member

aykevl commented Jun 27, 2019

A related function is implemented here in the runtime, which you can use as an example:

// indexByteString returns the index of the first instance of c in s, or -1 if c
// is not present in s.
//go:linkname indexByteString internal/bytealg.IndexByteString
func indexByteString(s string, c byte) int {
for i := 0; i < len(s); i++ {
if s[i] == c {
return i
}
}
return -1
}

You could either implement your own version, or copy the one from the Go standard library:
https://github.com/golang/go/blob/master/src/internal/bytealg/count_generic.go
But note that if you copy from the standard library, you have to clearly state where it came from.

@deadprogram deadprogram added the enhancement New feature or request label Jun 27, 2019
@1l0
Copy link
Contributor Author

1l0 commented Jun 27, 2019

I wonder if I have understood correctly.
Should following be compiled by TinyGo (actually failed) or do I have to compile TinyGo itself with a modification?

package main

import (
	"fmt"
	"strings"
	_ "unsafe"
)

func main() {
	s := strings.Replace("abc", "a", "x", -1)
	fmt.Println(s)
}

// paste from https://github.com/golang/go/blob/67f181bfd84dfd5942fe9a29d8a20c9ce5eb2fea/src/internal/bytealg/count_generic.go#L19
//go:linkname countString internal/bytealg.CountString
func countString(s string, c byte) int {
	n := 0
	for i := 0; i < len(s); i++ {
		if s[i] == c {
			n++
		}
	}
	return n
}

@aykevl
Copy link
Member

aykevl commented Jul 14, 2019

Sorry for the late reply.

Outside of the runtime, //go:linkname doesn't always work. This is a bug I'm aware of and should be fixed in the not too distant future hopefully (see #285). So simply moving this function to the runtime should get it to compile.

@1l0
Copy link
Contributor Author

1l0 commented Jul 15, 2019

OK I will wait the party.

@deadprogram
Copy link
Member

I think what @aykevl was saying @1l0 was that if you add that code to the correct place in the runtime will let it compile. You can then submit a PR and this should be solved.

The //go:linkname directive does not work on code outside of the runtime but works fine when used within the runtime.

Thanks!

@aykevl
Copy link
Member

aykevl commented Sep 18, 2019

Exactly, what @deadprogram says. There is no reason to wait, just move countString into the runtime to get it to compile (where it belongs anyway).

@1l0
Copy link
Contributor Author

1l0 commented Sep 19, 2019

Do you prefer that route (I mean patchwork-like)? I'm not hurrying so I can wait until fixing #285 that seems to fundamentally solve the problem.

@deadprogram
Copy link
Member

These seems some confusion.

The bug mentioned has nothing to do with the bytealg.CountString feature. it is that you cannot use the //go:linkname directive outside of the runtime package, which is not really related to the bytealg.CountString feature in this issue.

#285 has nothing directly to do with your issue.

I hope that clarifies.

@deadprogram
Copy link
Member

My own PR that implemented this based on your research @1l0 was merged into dev thanks for working on this. Now closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants