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.

Sign up for GitHub

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

add shared library support #429

Merged
merged 14 commits into from
Sep 28, 2023
Merged

add shared library support #429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
0a5a707
add shared library support
dicej May 16, 2023
481368b
generate separate .so files for emulated features
dicej Aug 9, 2023
4e7b184
revert errno changes in favor of a smaller change
dicej Aug 10, 2023
dbe2cb1
remove redundant `$(OBJDIR)/%.long-double.pic.o` rule in Makefile
dicej Aug 10, 2023
c651822
consolidate libwasi-emulated-*.so into a single library
dicej Aug 10, 2023
8c647d2
add comment explaining use of `--whole-archive`
dicej Aug 10, 2023
9bac3f9
Revert "remove redundant `$(OBJDIR)/%.long-double.pic.o` rule in Make…
dicej Aug 10, 2023
f303835
move `__main_void` from __main_void.c to crt1-command.c
dicej Aug 11, 2023
a7a552d
Revert "consolidate libwasi-emulated-*.so into a single library"
dicej Aug 15, 2023
62f25b2
build crt1-*.o with `-fPIC`
dicej Aug 15, 2023
167c890
ignore `__memory_base` when checking undefined symbols
dicej Aug 15, 2023
0c08d4b
Revert "move `__main_void` from __main_void.c to crt1-command.c"
dicej Aug 23, 2023
73d699e
add explanatory comments to __main_void.c
dicej Aug 23, 2023
cfc6fbc
add `__wasilibc_unmodified_upstream` and comment to `__lctrans_cur`
dicej Aug 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 79 additions & 9 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,6 +472,52 @@ endif

default: finish

LIBC_SO_OBJS = $(patsubst %.o,%.pic.o,$(filter-out $(MUSL_PRINTSCAN_OBJS),$(LIBC_OBJS)))
MUSL_PRINTSCAN_LONG_DOUBLE_SO_OBJS = $(patsubst %.o,%.pic.o,$(MUSL_PRINTSCAN_LONG_DOUBLE_OBJS))
LIBWASI_EMULATED_MMAN_SO_OBJS = $(patsubst %.o,%.pic.o,$(LIBWASI_EMULATED_MMAN_OBJS))
LIBWASI_EMULATED_PROCESS_CLOCKS_SO_OBJS = $(patsubst %.o,%.pic.o,$(LIBWASI_EMULATED_PROCESS_CLOCKS_OBJS))
LIBWASI_EMULATED_GETPID_SO_OBJS = $(patsubst %.o,%.pic.o,$(LIBWASI_EMULATED_GETPID_OBJS))
LIBWASI_EMULATED_SIGNAL_SO_OBJS = $(patsubst %.o,%.pic.o,$(LIBWASI_EMULATED_SIGNAL_OBJS))
LIBWASI_EMULATED_SIGNAL_MUSL_SO_OBJS = $(patsubst %.o,%.pic.o,$(LIBWASI_EMULATED_SIGNAL_MUSL_OBJS))
BULK_MEMORY_SO_OBJS = $(patsubst %.o,%.pic.o,$(BULK_MEMORY_OBJS))
DLMALLOC_SO_OBJS = $(patsubst %.o,%.pic.o,$(DLMALLOC_OBJS))
LIBC_BOTTOM_HALF_ALL_SO_OBJS = $(patsubst %.o,%.pic.o,$(LIBC_BOTTOM_HALF_ALL_OBJS))
LIBC_TOP_HALF_ALL_SO_OBJS = $(patsubst %.o,%.pic.o,$(LIBC_TOP_HALF_ALL_OBJS))

PIC_OBJS = \
$(LIBC_SO_OBJS) \
$(MUSL_PRINTSCAN_LONG_DOUBLE_SO_OBJS) \
$(LIBWASI_EMULATED_MMAN_SO_OBJS) \
$(LIBWASI_EMULATED_PROCESS_CLOCKS_SO_OBJS) \
$(LIBWASI_EMULATED_GETPID_SO_OBJS) \
$(LIBWASI_EMULATED_SIGNAL_SO_OBJS) \
$(LIBWASI_EMULATED_SIGNAL_MUSL_SO_OBJS) \
$(BULK_MEMORY_SO_OBJS) \
$(DLMALLOC_SO_OBJS) \
$(LIBC_BOTTOM_HALF_ALL_SO_OBJS) \
$(LIBC_TOP_HALF_ALL_SO_OBJS) \
$(LIBC_BOTTOM_HALF_CRT_OBJS)

# TODO: Specify SDK version, e.g. libc.so.wasi-sdk-21, as SO_NAME once `wasm-ld`
# supports it.
#
# Note that we collect the object files for each shared library into a .a and
# link that using `--whole-archive` rather than pass the object files directly
# to CC. This is a workaround for a Windows command line size limitation. See
# the `%.a` rule below for details.
$(SYSROOT_LIB)/%.so: $(OBJDIR)/%.so.a $(BUILTINS_LIB)
$(CC) -nostdlib -shared -o $@ -Wl,--whole-archive $< -Wl,--no-whole-archive $(BUILTINS_LIB)

$(OBJDIR)/libc.so.a: $(LIBC_SO_OBJS) $(MUSL_PRINTSCAN_LONG_DOUBLE_SO_OBJS)

$(OBJDIR)/libwasi-emulated-mman.so.a: $(LIBWASI_EMULATED_MMAN_SO_OBJS)

$(OBJDIR)/libwasi-emulated-process-clocks.so.a: $(LIBWASI_EMULATED_PROCESS_CLOCKS_SO_OBJS)

$(OBJDIR)/libwasi-emulated-getpid.so.a: $(LIBWASI_EMULATED_GETPID_SO_OBJS)

$(OBJDIR)/libwasi-emulated-signal.so.a: $(LIBWASI_EMULATED_SIGNAL_SO_OBJS) $(LIBWASI_EMULATED_SIGNAL_MUSL_SO_OBJS)
Copy link
Member

Choose a reason for hiding this comment

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

Why produce these intermediate .a archives and then link them with --whole-archive? i.e. Why not just add the objects to the link line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #429 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

This is kind of sad. Can you add a comment before the --whole-archive line to explain why we are doing this?

I seems like there must be a better solution to this windows issue :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a comment.


$(SYSROOT_LIB)/libc.a: $(LIBC_OBJS)

$(SYSROOT_LIB)/libc-printscan-long-double.a: $(MUSL_PRINTSCAN_LONG_DOUBLE_OBJS)
Expand All @@ -497,6 +543,8 @@ $(SYSROOT_LIB)/libwasi-emulated-signal.a: $(LIBWASI_EMULATED_SIGNAL_OBJS) $(LIBW
# silently dropping the tail.
$(AR) crs $@ $(wordlist 800, 100000, $(sort $^))

$(PIC_OBJS): CFLAGS += -fPIC -fvisibility=default

$(MUSL_PRINTSCAN_OBJS): CFLAGS += \
-D__wasilibc_printscan_no_long_double \
-D__wasilibc_printscan_full_support_option="\"add -lc-printscan-long-double to the link command\""
Expand All @@ -507,15 +555,23 @@ $(MUSL_PRINTSCAN_NO_FLOATING_POINT_OBJS): CFLAGS += \

# TODO: apply -mbulk-memory globally, once
# https://github.com/llvm/llvm-project/issues/52618 is resolved
$(BULK_MEMORY_OBJS): CFLAGS += \
$(BULK_MEMORY_OBJS) $(BULK_MEMORY_SO_OBJS): CFLAGS += \
-mbulk-memory

$(BULK_MEMORY_OBJS): CFLAGS += \
$(BULK_MEMORY_OBJS) $(BULK_MEMORY_SO_OBJS): CFLAGS += \
-DBULK_MEMORY_THRESHOLD=$(BULK_MEMORY_THRESHOLD)

$(LIBWASI_EMULATED_SIGNAL_MUSL_OBJS): CFLAGS += \
$(LIBWASI_EMULATED_SIGNAL_MUSL_OBJS) $(LIBWASI_EMULATED_SIGNAL_MUSL_SO_OBJS): CFLAGS += \
-D_WASI_EMULATED_SIGNAL

$(OBJDIR)/%.long-double.pic.o: %.c include_dirs
@mkdir -p "$(@D)"
$(CC) $(CFLAGS) -MD -MP -o $@ -c $<

$(OBJDIR)/%.pic.o: %.c include_dirs
@mkdir -p "$(@D)"
$(CC) $(CFLAGS) -MD -MP -o $@ -c $<

$(OBJDIR)/%.long-double.o: %.c include_dirs
@mkdir -p "$(@D)"
$(CC) $(CFLAGS) -MD -MP -o $@ -c $<
Expand All @@ -534,17 +590,17 @@ $(OBJDIR)/%.o: %.s include_dirs

-include $(shell find $(OBJDIR) -name \*.d)

$(DLMALLOC_OBJS): CFLAGS += \
$(DLMALLOC_OBJS) $(DLMALLOC_SO_OBJS): CFLAGS += \
-I$(DLMALLOC_INC)

startup_files $(LIBC_BOTTOM_HALF_ALL_OBJS): CFLAGS += \
startup_files $(LIBC_BOTTOM_HALF_ALL_OBJS) $(LIBC_BOTTOM_HALF_ALL_SO_OBJS): CFLAGS += \
-I$(LIBC_BOTTOM_HALF_HEADERS_PRIVATE) \
-I$(LIBC_BOTTOM_HALF_CLOUDLIBC_SRC_INC) \
-I$(LIBC_BOTTOM_HALF_CLOUDLIBC_SRC) \
-I$(LIBC_TOP_HALF_MUSL_SRC_DIR)/include \
-I$(LIBC_TOP_HALF_MUSL_SRC_DIR)/internal

$(LIBC_TOP_HALF_ALL_OBJS) $(MUSL_PRINTSCAN_LONG_DOUBLE_OBJS) $(MUSL_PRINTSCAN_NO_FLOATING_POINT_OBJS) $(LIBWASI_EMULATED_SIGNAL_MUSL_OBJS): CFLAGS += \
$(LIBC_TOP_HALF_ALL_OBJS) $(LIBC_TOP_HALF_ALL_SO_OBJS) $(MUSL_PRINTSCAN_LONG_DOUBLE_OBJS) $(MUSL_PRINTSCAN_LONG_DOUBLE_SO_OBJS) $(MUSL_PRINTSCAN_NO_FLOATING_POINT_OBJS) $(LIBWASI_EMULATED_SIGNAL_MUSL_OBJS) $(LIBWASI_EMULATED_SIGNAL_MUSL_SO_OBJS): CFLAGS += \
-I$(LIBC_TOP_HALF_MUSL_SRC_DIR)/include \
-I$(LIBC_TOP_HALF_MUSL_SRC_DIR)/internal \
-I$(LIBC_TOP_HALF_MUSL_DIR)/arch/wasm32 \
Expand All @@ -558,7 +614,7 @@ $(LIBC_TOP_HALF_ALL_OBJS) $(MUSL_PRINTSCAN_LONG_DOUBLE_OBJS) $(MUSL_PRINTSCAN_NO
-Wno-dangling-else \
-Wno-unknown-pragmas

$(LIBWASI_EMULATED_PROCESS_CLOCKS_OBJS): CFLAGS += \
$(LIBWASI_EMULATED_PROCESS_CLOCKS_OBJS) $(LIBWASI_EMULATED_PROCESS_CLOCKS_SO_OBJS): CFLAGS += \
-I$(LIBC_BOTTOM_HALF_CLOUDLIBC_SRC)

# emmalloc uses a lot of pointer type-punning, which is UB under strict aliasing,
Expand Down Expand Up @@ -596,6 +652,20 @@ startup_files: include_dirs $(LIBC_BOTTOM_HALF_CRT_OBJS)
mkdir -p "$(SYSROOT_LIB)" && \
cp $(LIBC_BOTTOM_HALF_CRT_OBJS) "$(SYSROOT_LIB)"

# TODO: As of this writing, wasi_thread_start.s uses non-position-independent
# code, and I'm not sure how to make it position-independent. Once we've done
# that, we can enable libc.so for the wasi-threads build.
ifneq ($(THREAD_MODEL), posix)
LIBC_SO = \
$(SYSROOT_LIB)/libc.so \
$(SYSROOT_LIB)/libwasi-emulated-mman.so \
$(SYSROOT_LIB)/libwasi-emulated-process-clocks.so \
$(SYSROOT_LIB)/libwasi-emulated-getpid.so \
$(SYSROOT_LIB)/libwasi-emulated-signal.so
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure having 5 separate shared libraries here is the right way to go.

Could we maybe start by simply not included these optional parts in the shared library build? For now they could continue to be linked in statically if needed, and we can maybe come up with a more advanced solution later, as needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'd still need to build separate libraries using -fPIC (e.g. libwasi-emulated-mman-pic.a). Or I guess if folks are okay with using -fPIC unconditionally for the relevant .o files, we could do that.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking the they would be linked into the main binary and wouldn't need -fPIC. But that would limit their usefulness I guess. This approach just seems a little overly complex, which is why I suggested punting on this for a while.

An alternative approach, for example, would be to have a single libc-emulated.so or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, libraries sometimes need these features, too, so I wouldn't want to restrict them to the main binary.

I'd be in favor of a single libc-emulated.so. @yamt would that be okay with you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went ahead and combined the libraries into a single libwasi-emulated.so.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be in favor of a single libc-emulated.so. @yamt would that be okay with you?

i'm not sure if it's a good idea.
it would make try-link style configure scripts detect all of the emulated stuff
once you link the single libc-emulated.so.
also, what to do about _WASI_EMULATED_xxx macros?
i feel it's conservative to mirror what we do for static libs at least for the first step.

Copy link
Contributor Author

@dicej dicej Aug 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

To sum up the conversation so far:

  • Initially, I bundled everything into libc.so for simplicity
  • @yamt prefers we follow the pattern used for the static libraries and build a separate libwasi-emulated-*.so corresponding to each libwasi-emulated-*.a file
  • @sbc100 is concerned that building so many .so files is overly complex and would prefer a single libwasi-emulated.so

Personally, I just want to get this PR merged, so I'm fine with any of the above. If the maintainers of this project can come to a decision on this, I'm happy to implement any changes required.

Copy link
Member

Choose a reason for hiding this comment

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

I don't feel strongly about the multiple .so files and in fact I can see value in mirroring the .a names. Sorry for the noise.

I wish we didn't need all those special .so files but I guess since we decided to make the emulation optional we don't have much choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @sbc100. I've reverted the change the combined the libwasi-emulated-*.so into a single library, so we're back to four separate ones.

endif

libc_so: include_dirs $(LIBC_SO)
Copy link
Contributor

Choose a reason for hiding this comment

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

is this the main target to build shared library?
which target builds pic version of crt1*.o?

Copy link
Contributor Author

@dicej dicej Aug 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

is this the main target to build shared library?

Yes

which target builds pic version of crt1*.o?

My understanding is that crt1.o is intended to be linked into the main module, which doesn't necessarily need to be PIC since it's the one exporting memory, not importing it. I'll admit I've been focusing on reactor-style apps, though, so if there's something missing to support command-style apps I'm happy to add it.

Copy link
Contributor

@yamt yamt Aug 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

which target builds pic version of crt1*.o?

My understanding is that crt1.o is intended to be linked into the main module, which doesn't necessarily need to be PIC since it's the one exporting memory, not importing it. I'll admit I've been focusing on reactor-style apps, though, so if there's something missing to support command-style apps I'm happy to add it.

i was assuming that -pie was used to produce a dynamically linked executable.
https://github.com/WebAssembly/tool-conventions/blob/main/DynamicLinking.md#llvm-implementation
but as you say, it might be possible to produce a non-pie dynamically linked executable.
i haven't tried.

btw, a reactor module is linked with crt1-reactor.o as well.
i guess you meant shared libraries, for which if crt1-reactor.o should be linked isn't obvious.

Copy link
Contributor

Choose a reason for hiding this comment

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

but as you say, it might be possible to produce a non-pie dynamically linked executable.
i haven't tried.

it seems clang can somehow produce non-pie dynamically linked main module.
eg.

-fPIC
-nodefaultlibs
-Xlinker --unresolved-symbols=import-dynamic
-Xlinker --export-table
-Xlinker --growable-table

a few things are not obvious to me though:

  • what to do for __stack_pointer. should the main module export it? or import it?
  • how about __heap_base and __heap_end?

on the other hand, pie main module is more straightforward and known to work as it's what emscripten -sMAIN_MODULE uses.
to produce pie main modules, we need pic (or at least dynamic-no-pic) version of crt1.

how do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I believe the main module must export __stack_pointer, __heap_base, and __heap_end (or at least some module must export them, because shared libraries will import them in general).

I'd be fine with building crt1-*.o with -fPIC unconditionally -- it's a very small amount of code, so I doubt there will be a noticeable performance penalty. Does anyone have concerns about that?

Copy link
Member

@sbc100 sbc100 Aug 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think crt1.o (or something like it) will always be needed in order to define the _initialize function (who's primary job, at least in the llvm world is to call __wasm_call_ctors).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think crt1.o (or something like it) will always be needed in order to define the _initialize function (who's primary job, at least in the llvm world is to call __wasm_call_ctors).

wasm-tools component link also synthesizes an "init" module which contains a start function that calls __wasm_apply_data_relocs, __wasm_call_ctors, initializes GOT globals, etc.

Copy link
Member

Choose a reason for hiding this comment

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

I think in the long run we should use something like _initialize exported rather than __wasm_call_ctors (which I see as an llvm-specific thing).

__wasm_apply_data_relocs is a tricky one because (at least in cyclic module graphs) we need to call that on the whole module graph before we call __wasm_call_ctors on the whole module graph.. so perhaps we need to standardize on some kind of phase 1 and phase 2 _initialize functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've pushed an update to build crt1-*.o with -fPIC.

Copy link
Contributor

Choose a reason for hiding this comment

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

As a data point, emscripten currently always builds the main module as -pie and then the runtime provides things like __stack_pointer, __heap_end, etc.

toywasm libdyld does the same.

I've been trying for a while to convert emscripten to use statically linked main modules but I have not had any success yet. See emscripten-core/emscripten#12682

here's my similar attempt: yamt/toywasm#112


libc: include_dirs \
$(SYSROOT_LIB)/libc.a \
$(SYSROOT_LIB)/libc-printscan-long-double.a \
Expand Down Expand Up @@ -645,7 +715,7 @@ check-symbols: startup_files libc
for undef_sym in $$("$(NM)" --undefined-only "$(SYSROOT_LIB)"/libc.a "$(SYSROOT_LIB)"/libc-*.a "$(SYSROOT_LIB)"/*.o \
|grep ' U ' |sed 's/.* U //' |LC_ALL=C sort |uniq); do \
grep -q '\<'$$undef_sym'\>' "$(DEFINED_SYMBOLS)" || echo $$undef_sym; \
done | grep -v "^__mul" > "$(UNDEFINED_SYMBOLS)"
done | grep -E -v "^__mul|__memory_base" > "$(UNDEFINED_SYMBOLS)"
grep '^_*imported_wasi_' "$(UNDEFINED_SYMBOLS)" \
> "$(SYSROOT_LIB)/libc.imports"

Expand Down Expand Up @@ -728,4 +798,4 @@ clean:
$(RM) -r "$(OBJDIR)"
$(RM) -r "$(SYSROOT)"

.PHONY: default startup_files libc finish install include_dirs clean
.PHONY: default startup_files libc libc_so finish install include_dirs clean check-symbols
1 change: 0 additions & 1 deletion expected/wasm32-wasi-threads/undefined-symbols.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ __imported_wasi_snapshot_preview1_sock_shutdown
__imported_wasi_thread_spawn
__letf2
__lttf2
__main_argc_argv
__netf2
__stack_pointer
__subtf3
Expand Down
1 change: 0 additions & 1 deletion expected/wasm32-wasi/undefined-symbols.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ __imported_wasi_snapshot_preview1_sock_send
__imported_wasi_snapshot_preview1_sock_shutdown
__letf2
__lttf2
__main_argc_argv
__netf2
__stack_pointer
__subtf3
Expand Down
5 changes: 0 additions & 5 deletions libc-bottom-half/headers/public/__errno.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,11 @@
extern "C" {
#endif

#ifdef __cplusplus
extern thread_local int errno;
#else
extern _Thread_local int errno;
#endif

#define errno errno

#ifdef __cplusplus
}
#endif

#endif
11 changes: 11 additions & 0 deletions libc-bottom-half/sources/__main_void.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,22 @@
#include <sysexits.h>

// The user's `main` function, expecting arguments.
//
// Note that we make this a weak symbol so that it will have a
// `WASM_SYM_BINDING_WEAK` flag in libc.so, which tells the dynamic linker that
// it need not be defined (e.g. in reactor-style apps with no main function).
// See also the TODO comment on `__main_void` below.
__attribute__((__weak__))
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed in particular for the shared build? ? Perhaps add a comment?

Copy link
Member

Choose a reason for hiding this comment

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

... I wonder if this file should be logically part of crt1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reference, this is the issue that prompted the change: dicej/component-linking-demo#2. In a nutshell: we don't want to force reactor-style apps to define it.

Yes, putting it in crt1 makes sense to me. Would that entail moving it to libc-top-half/musl/crt/crt1.c or similar?

cc @sunfishcode

Copy link
Member

Choose a reason for hiding this comment

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

wasi-libc doesn't use musl's crt1.c; it uses libc-bottom-half/crt/crt1-command.c and libc-bottom-half/crt/crt1-reactor.c. Putting the __main_argc_argc symbol there sounds reasonable to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've moved both the __main_void definition and the __main_argc_argv declaration to crt1-command.c.

int __main_argc_argv(int argc, char *argv[]);

// If the user's `main` function expects arguments, the compiler will rename
// it to `__main_argc_argv`, and this version will get linked in, which
// initializes the argument data and calls `__main_argc_argv`.
//
// TODO: Ideally this function would be defined in a crt*.o file and linked in
// as necessary by the Clang driver. However, moving it to crt1-command.c
// breaks `--no-gc-sections`, so we'll probably need to create a new file
// (e.g. crt0.o or crtend.o) and teach Clang to use it when needed.
__attribute__((__weak__, nodebug))
int __main_void(void) {
__wasi_errno_t err;
Expand Down
8 changes: 8 additions & 0 deletions libc-top-half/musl/src/internal/locale_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,15 @@ extern hidden const struct __locale_struct __c_dot_utf8_locale;
hidden const struct __locale_map *__get_locale(int, const char *);
hidden const char *__mo_lookup(const void *, size_t, const char *);
hidden const char *__lctrans(const char *, const struct __locale_map *);
#ifdef __wasilibc_unmodified_upstream
hidden const char *__lctrans_cur(const char *);
#else
// We make this visible in the wasi-libc build because
// libwasi-emulated-signal.so needs to import it from libc.so. If we ever
// decide to merge libwasi-emulated-signal.so into libc.so, this will no longer
// be necessary.
const char *__lctrans_cur(const char *);
Copy link
Contributor

Choose a reason for hiding this comment

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

is this intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - libc-top-half/musl/src/string/strsignal.c (which is included in LIBWASI_EMULATED_SIGNAL_MUSL_SOURCES) uses LCTRANS_CUR (which is defined to be __lctrans_cur), which means libc.so must export it so libwasi-emulated.so can import it.

Copy link
Member

Choose a reason for hiding this comment

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

I think this warrents and #ifdef __wasilibc_unmodified_upstream along with a comment.

This is another reason why splitting libc into multiple libraries is maybe not the best idea.

#endif
hidden const char *__lctrans_impl(const char *, const struct __locale_map *);
hidden int __loc_is_allocated(locale_t);
hidden char *__gettextdomain(void);
Expand Down