Skip to content

Implement some string functions for libzigc #23847

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

RcCreeperTech
Copy link

@RcCreeperTech RcCreeperTech commented May 10, 2025

Progress towards #2879.

  • Add memchr, memrchr, strchr, strchrnul, strrchr, index, rindex, memset, memccpy, memmem, mempcpy, stpcpy, stpncpy, strcat, strcpy, strncpy, strnlen
  • Remove memcmp and depend on compiler_rt implementation

- Add memchr, memrchr, strchr, strchrnul, strrchr
- Remove memcmp and depend on compiler_rt implementation
Add index, rindex, memset
strrchr will return the address of the null terminator it c == 0
- add memccpy, memmem, mempcpy, stpcpy, stpncpy, strcat, strcpy, strncpy, strnlen
Copy link
Contributor

@rpkak rpkak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about creating a PR with functions like this a few weeks ago but decided not to start working on it until #23538 is resolved because I don't want to make a nearly unnoticeable diversion between libzigc and libc. Since this PR exists now, I tested it against "libc-test/src/functional/string*.c" and found the following error:

src/functional/string_strchr.c:73: strchr(s,128) with align=0 returned 0, wanted str+127
src/functional/string_strchr.c:73: strchr(s,128) with align=1 returned 0, wanted str+127
src/functional/string_strchr.c:73: strchr(s,128) with align=2 returned 0, wanted str+127
src/functional/string_strchr.c:73: strchr(s,128) with align=3 returned 0, wanted str+127
src/functional/string_strchr.c:73: strchr(s,128) with align=4 returned 0, wanted str+127
src/functional/string_strchr.c:73: strchr(s,128) with align=5 returned 0, wanted str+127
src/functional/string_strchr.c:73: strchr(s,128) with align=6 returned 0, wanted str+127
src/functional/string_strchr.c:73: strchr(s,128) with align=7 returned 0, wanted str+127
src/functional/string_strchr.c:74: strchr(s,255) with align=0 returned 0, wanted str+254
src/functional/string_strchr.c:74: strchr(s,255) with align=1 returned 0, wanted str+254
src/functional/string_strchr.c:74: strchr(s,255) with align=2 returned 0, wanted str+254
src/functional/string_strchr.c:74: strchr(s,255) with align=3 returned 0, wanted str+254
src/functional/string_strchr.c:74: strchr(s,255) with align=4 returned 0, wanted str+254
src/functional/string_strchr.c:74: strchr(s,255) with align=5 returned 0, wanted str+254
src/functional/string_strchr.c:74: strchr(s,255) with align=6 returned 0, wanted str+254
src/functional/string_strchr.c:74: strchr(s,255) with align=7 returned 0, wanted str+254

It is caused by @as(c_char, @bitCast(@as(u8, 128))) == -128 != 128.

- Correct pointer types for alignment
- Use stdlib functions for `memchr` and `memrchr`
- Remove `mempcpy` and `strnlen` implementaitons from `lib/libc/mingw`
- Remove duplicate `__aeabi_mem*` symbols
@alexrp
Copy link
Member

alexrp commented May 12, 2025

Has conflicts after #23835 due to files being moved; a rebase should be enough to deal with it automatically.

Comment on lines +26 to +29
@export(&stpncpy, .{ .name = "__stpncpy", .linkage = .weak, .visibility = .hidden });
@export(&strchrnul, .{ .name = "__strchrnul", .linkage = .weak, .visibility = .hidden });
@export(&memrchr, .{ .name = "__memrchr", .linkage = .weak, .visibility = .hidden });
@export(&stpcpy, .{ .name = "__stpcpy", .linkage = .weak, .visibility = .hidden });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I did not realize that these are hidden symbols. In that case, we can just remove them altogether.

For future reference, lib/libc/musl/libc.S is a good reference for which symbols are actually part of the musl ABI and thus should be provided by libzigc.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is fine with me. I only tried to export these symbols because I was getting linker errors when they were removed if I am remembering correctly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh? Do you recall the linker errors? Maybe there's something I'm missing here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember exactly since it was a few days ago, I just removed them again and I am re-running the test suite locally so we will see if the errors re-appear.

@alexrp
Copy link
Member

alexrp commented May 12, 2025

In hindsight, the way I set things up in lib/c.zig didn't really make sense:

zig/lib/c.zig

Lines 16 to 34 in 833d4c9

comptime {
if (builtin.target.isMuslLibC() or builtin.target.isWasiLibC()) {
// Files specific to musl and wasi-libc.
_ = @import("c/string.zig");
_ = @import("c/strings.zig");
}
if (builtin.target.isMuslLibC()) {
// Files specific to musl.
}
if (builtin.target.isWasiLibC()) {
// Files specific to wasi-libc.
}
if (builtin.target.isMinGW()) {
// Files specific to MinGW-w64.
}
}

I suggest you move those two @imports above any of the libc-specific blocks, and instead cover the @exports with appropriate builtin.target.is*LibC() checks in lib/c/string[s].zig. In particular, most of the exports aren't needed for MinGW - only mempcpy and strnlen at this time, AFAICT.

@export(&strncmp, .{ .name = "strncmp", .linkage = common.linkage, .visibility = common.visibility });
@export(&strchr, .{ .name = "strchr", .linkage = common.linkage, .visibility = common.visibility });
@export(&strchr, .{ .name = "index", .linkage = common.linkage, .visibility = common.visibility });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

index and rindex are part of strings.h and so should go into strings.zig (you'll need to rebase on upstream master for that).

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

Successfully merging this pull request may close these issues.

5 participants