src/os: make Environ() return a copy of the environment #2647
Conversation
|
this pull request should also bring back the test deleted by #2603 |
Removed as flaky in tinygo-org#2603
src/os/env.go
Outdated
| func Environ() []string { | ||
| return syscall.Environ() | ||
| orig := syscall.Environ() | ||
| single := strings.Join(orig, "") |
There was a problem hiding this comment.
Upsteam does the copy in syscall.Environ(), we might want to do the same in case someone calls that directly?
|
That probably makes sense. I'll move this logic (or equivalent) there. |
|
Would it make sense to do a single allocation to copy libc_environ and then point to bits of it? That might be faster...? |
|
Yeah, I was torn about doing that because we need to loop over the environment a few times instead of just once. Let me try that approach and see what it looks like |
|
Now with a single allocation. |
dgryski
force-pushed
the
dgryski/env-copy
branch
from
b62f37f to
b235c9d
Compare
dgryski
force-pushed
the
dgryski/env-copy
branch
from
b235c9d to
72ca05f
Compare
| // copy over the bytes to the Go heap | ||
| copy(bs, envVar) | ||
| // convert trimmed slice to string | ||
| s := *(*string)(unsafe.Pointer(&bs)) |
There was a problem hiding this comment.
This is a dirty trick... but I have to admit, this whole function isn't exactly clean and I wrote it originally.
(Dirty because it's not guaranteed that slice/string layout will stay the way it is now - although I don't see why it would need to change).
Note that this can have the side effect of keeping more alive than needed. Imagine you only need a single value out of the environment. So you call |
|
Good point about a single environment variable pinning the entire allocation. I think I'll add a comment to this function about that |
|
Yeah, that went through my head too, I should have mentioned it. |
4 participants
Fixes #2646