libzigc: strdup, strndup, wcsdup #31144

the std.mem.span is redundant here, no?

You're right, thanks!

??

It's because we test libzigc without linking libc, so the malloc call below would be a compile error---but this is a hacky solution and also makes these functions untestable, so I think needs to be changed. Seems to me that we should just test libzigc with libc linked, although that could lead to weirdness due to there being two copies of these functions...

Both hacks are unacceptable to me. The simplest way to tackle this problem is to treat malloc as a dependency of this function, and therefore it must be implemented first.

I called the second thing a hack, but it's not a hack, it's the only correct way to test this code. The libc when targeting windows-gnu for example, provides some libc functionality but requires calling functions from ucrtbase.dll, including _errno for example.

There won't be any "weirdness due to there being two copies of these functions" because the extra copy will be deleted along with providing the Zig implementation.

Anyway we have malloc now after #31177

Hmm actually there is indeed a problem. What we want is to run zig test lib/c.zig ... with all the additional libc components added (i.e. not yet ported musl .c files). However, the logic that knows how to set up that command line is using the Compilation API directly, in musl.zig, mingw.zig, etc.

Instead, what we get if we adjust the build script to link libc, is lib/c.zig acting as the user application, with an additional copy of lib/c.zig + companion files, just like @mlugg pointed out.

So we either have to special case the zig libc unit test compilation unit, which I am loathe to do, or come up with another unit testing strategy, such as testing libc from outside the ABI boundary.

A good solution would be doing what compiler_rt does for unit testing, which is set the symbol linkage to "internal" when builtin.is_test is true. That is currently not done in zig libc because sometimes zig libc is imported directly into the Zig Compilation Unit of the user application, which might be a unit test.

This leads me to a solution: we can set symbol linkage to internal if it's a test and lib/c.zig is the root source file.

That condition is not currently detectable, because the root module (@import("root")) in a test is the test runner, not the module being tested.

Dammit I was so close:

-/// Determines the symbol's visibility to other objects.
-/// For WebAssembly this allows the symbol to be resolved to other modules, but will not
-/// export it to the host runtime.
-pub const visibility: std.builtin.SymbolVisibility = .hidden;
+/// It is possible that this libc is being linked into a different test
+/// compilation, as opposed to being tested itself. In such case,
+/// `builtin.link_libc` will be `true` along with `builtin.is_test`.
+///
+/// When we don't have a complete libc, `builtin.link_libc` will be `false` and
+/// we will be missing externally provided symbols, such as `_errno` from
+/// ucrtbase.dll. In such case, we must avoid analyzing otherwise exported
+/// functions because it would cause undefined symbol usage.
+const skip_export = builtin.is_test and !builtin.link_libc;
 
 pub inline fn symbol(comptime func: *const anyopaque, comptime name: []const u8) void {
- // Normally, libc goes into a static archive, making all symbols
- // overridable. However, Zig supports including the libc functions as part
- // of the Zig Compilation Unit, so to support this use case we make all
- // symbols weak.
- @export(func, .{ .name = name, .linkage = .weak, .visibility = visibility });
+ if (skip_export) return;
+
+ @export(func, .{
+ .name = name,
+ // Normally, libc goes into a static archive, making all symbols
+ // overridable. However, Zig supports including the libc functions as part
+ // of the Zig Compilation Unit, so to support this use case we make all
+ // symbols weak.
+ .linkage = .weak,
+ // For WebAssembly, hidden visibility allows the symbol to be resolved to
+ // other modules, but will not export it to the host runtime.
+ .visibility = .hidden,
+ });
 }

It doesn't work because symbol(&foo, "foo") still causes foo to be analyzed, even when skip_export is true.

cant you just do comptime func: anytype and take the address inside the function behind the condition?

No it still gets typed checked in that case. Plus there's a lack of type safety there (if you pass &func it silently does the wrong thing). I can just move the logic to the comptime block.

thats somewhat surprising to me that it still gets checked, i wouldnt expect that to happen since the & isnt reached. good point on type safety though

calculates length 2x instead of 1x

Do not call strndup anymore on 502a92a44b to calculate length only 1x. Sorry for my english