| Message ID | 9da41d784991f77e2c1f38d0781cd047b593e053.1744787186.git.ktokunaga.mail@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enable QEMU TCI to run 32bit guests on browsers | expand |
On 16/4/25 10:14, Kohei Tokunaga wrote: > emscripten exposes copy_file_range declaration but doesn't provide the > implementation in the final link. Define the emscripten-specific stub > function to avoid type conflict with the emscripten's header. > > Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com> > --- > block/file-posix.c | 6 ++++++ > stubs/emscripten.c | 13 +++++++++++++ > 2 files changed, 19 insertions(+) > create mode 100644 stubs/emscripten.c > > diff --git a/block/file-posix.c b/block/file-posix.c > index 56d1972d15..22e0ed5069 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -110,6 +110,10 @@ > #include <sys/diskslice.h> > #endif > > +#ifdef EMSCRIPTEN > +#include <sys/ioctl.h> > +#endif > + > /* OS X does not have O_DSYNC */ > #ifndef O_DSYNC > #ifdef O_SYNC > @@ -2010,6 +2014,7 @@ static int handle_aiocb_write_zeroes_unmap(void *opaque) > return handle_aiocb_write_zeroes(aiocb); > } > > +#ifndef EMSCRIPTEN > #ifndef HAVE_COPY_FILE_RANGE > static off_t copy_file_range(int in_fd, off_t *in_off, int out_fd, > off_t *out_off, size_t len, unsigned int flags) > @@ -2023,6 +2028,7 @@ static off_t copy_file_range(int in_fd, off_t *in_off, int out_fd, > #endif > } > #endif > +#endif > > /* > * parse_zone - Fill a zone descriptor > diff --git a/stubs/emscripten.c b/stubs/emscripten.c > new file mode 100644 > index 0000000000..2157d6349b > --- /dev/null > +++ b/stubs/emscripten.c > @@ -0,0 +1,13 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +#include "qemu/osdep.h" > +/* > + * emscripten exposes copy_file_range declaration but doesn't provide the > + * implementation in the final link. Define the stub here but avoid type > + * conflict with the emscripten's header. > + */ > +ssize_t copy_file_range(int in_fd, off_t *in_off, int out_fd, > + off_t *out_off, size_t len, unsigned int flags) > +{ > + errno = ENOSYS; > + return -1; > +} I'd include in this patch this hunk from patch 17: -- >8 -- diff --git a/stubs/meson.build b/stubs/meson.build index 63392f5e78..4fd4d362f9 100644 --- a/stubs/meson.build +++ b/stubs/meson.build @@ -89,3 +89,7 @@ if have_system or have_user stub_ss.add(files('hotplug-stubs.c')) stub_ss.add(files('sysbus.c')) endif + +if host_os == 'emscripten' + stub_ss.add(files('emscripten.c')) +endif ---
On 16/4/25 11:00, Philippe Mathieu-Daudé wrote: > On 16/4/25 10:14, Kohei Tokunaga wrote: >> emscripten exposes copy_file_range declaration but doesn't provide the >> implementation in the final link. Define the emscripten-specific stub >> function to avoid type conflict with the emscripten's header. >> >> Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com> >> --- >> block/file-posix.c | 6 ++++++ >> stubs/emscripten.c | 13 +++++++++++++ >> 2 files changed, 19 insertions(+) >> create mode 100644 stubs/emscripten.c >> >> diff --git a/block/file-posix.c b/block/file-posix.c >> index 56d1972d15..22e0ed5069 100644 >> --- a/block/file-posix.c >> +++ b/block/file-posix.c >> @@ -110,6 +110,10 @@ >> #include <sys/diskslice.h> >> #endif >> +#ifdef EMSCRIPTEN >> +#include <sys/ioctl.h> >> +#endif >> + >> /* OS X does not have O_DSYNC */ >> #ifndef O_DSYNC >> #ifdef O_SYNC >> @@ -2010,6 +2014,7 @@ static int handle_aiocb_write_zeroes_unmap(void >> *opaque) >> return handle_aiocb_write_zeroes(aiocb); >> } >> +#ifndef EMSCRIPTEN >> #ifndef HAVE_COPY_FILE_RANGE >> static off_t copy_file_range(int in_fd, off_t *in_off, int out_fd, >> off_t *out_off, size_t len, unsigned >> int flags) >> @@ -2023,6 +2028,7 @@ static off_t copy_file_range(int in_fd, off_t >> *in_off, int out_fd, >> #endif >> } >> #endif >> +#endif >> /* >> * parse_zone - Fill a zone descriptor >> diff --git a/stubs/emscripten.c b/stubs/emscripten.c >> new file mode 100644 >> index 0000000000..2157d6349b >> --- /dev/null >> +++ b/stubs/emscripten.c >> @@ -0,0 +1,13 @@ >> +/* SPDX-License-Identifier: GPL-2.0-or-later */ >> +#include "qemu/osdep.h" >> +/* >> + * emscripten exposes copy_file_range declaration but doesn't provide >> the >> + * implementation in the final link. Define the stub here but avoid type >> + * conflict with the emscripten's header. >> + */ >> +ssize_t copy_file_range(int in_fd, off_t *in_off, int out_fd, >> + off_t *out_off, size_t len, unsigned int >> flags) >> +{ >> + errno = ENOSYS; >> + return -1; >> +} > > I'd include in this patch this hunk from patch 17: > > -- >8 -- > diff --git a/stubs/meson.build b/stubs/meson.build > index 63392f5e78..4fd4d362f9 100644 > --- a/stubs/meson.build > +++ b/stubs/meson.build > @@ -89,3 +89,7 @@ if have_system or have_user > stub_ss.add(files('hotplug-stubs.c')) > stub_ss.add(files('sysbus.c')) > endif > + > +if host_os == 'emscripten' > + stub_ss.add(files('emscripten.c')) > +endif > --- Actually what about checking the symbol presence in meson? Something like (untested): -- >8 -- diff --git a/meson.build b/meson.build index b18c46d16a2..33185fdf315 100644 --- a/meson.build +++ b/meson.build @@ -2654,3 +2654,2 @@ config_host_data.set('CONFIG_TIMERFD', cc.has_function('timerfd_create')) config_host_data.set('CONFIG_GETLOADAVG', cc.has_function('getloadavg')) -config_host_data.set('HAVE_COPY_FILE_RANGE', cc.has_function('copy_file_range')) config_host_data.set('HAVE_GETIFADDRS', cc.has_function('getifaddrs')) @@ -2756,2 +2755,6 @@ config_host_data.set('HAVE_UTMPX', +config_host_data.set('HAVE_COPY_FILE_RANGE', cc.links(gnu_source_prefix + ''' + #include <unistd.h> + int main(void) { return copy_file_range(-1, NULL, -1, NULL, 0, 0); }''')) config_host_data.set('CONFIG_EVENTFD', cc.links(''' ---
On 4/16/25 02:33, Philippe Mathieu-Daudé wrote: > On 16/4/25 11:00, Philippe Mathieu-Daudé wrote: >> On 16/4/25 10:14, Kohei Tokunaga wrote: >>> emscripten exposes copy_file_range declaration but doesn't provide the >>> implementation in the final link. Define the emscripten-specific stub >>> function to avoid type conflict with the emscripten's header. >>> >>> Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com> >>> --- >>> block/file-posix.c | 6 ++++++ >>> stubs/emscripten.c | 13 +++++++++++++ >>> 2 files changed, 19 insertions(+) >>> create mode 100644 stubs/emscripten.c >>> >>> diff --git a/block/file-posix.c b/block/file-posix.c >>> index 56d1972d15..22e0ed5069 100644 >>> --- a/block/file-posix.c >>> +++ b/block/file-posix.c >>> @@ -110,6 +110,10 @@ >>> #include <sys/diskslice.h> >>> #endif >>> +#ifdef EMSCRIPTEN >>> +#include <sys/ioctl.h> >>> +#endif >>> + >>> /* OS X does not have O_DSYNC */ >>> #ifndef O_DSYNC >>> #ifdef O_SYNC >>> @@ -2010,6 +2014,7 @@ static int handle_aiocb_write_zeroes_unmap(void *opaque) >>> return handle_aiocb_write_zeroes(aiocb); >>> } >>> +#ifndef EMSCRIPTEN >>> #ifndef HAVE_COPY_FILE_RANGE >>> static off_t copy_file_range(int in_fd, off_t *in_off, int out_fd, >>> off_t *out_off, size_t len, unsigned int flags) >>> @@ -2023,6 +2028,7 @@ static off_t copy_file_range(int in_fd, off_t *in_off, int out_fd, >>> #endif >>> } >>> #endif >>> +#endif >>> /* >>> * parse_zone - Fill a zone descriptor >>> diff --git a/stubs/emscripten.c b/stubs/emscripten.c >>> new file mode 100644 >>> index 0000000000..2157d6349b >>> --- /dev/null >>> +++ b/stubs/emscripten.c >>> @@ -0,0 +1,13 @@ >>> +/* SPDX-License-Identifier: GPL-2.0-or-later */ >>> +#include "qemu/osdep.h" >>> +/* >>> + * emscripten exposes copy_file_range declaration but doesn't provide the >>> + * implementation in the final link. Define the stub here but avoid type >>> + * conflict with the emscripten's header. >>> + */ >>> +ssize_t copy_file_range(int in_fd, off_t *in_off, int out_fd, >>> + off_t *out_off, size_t len, unsigned int flags) >>> +{ >>> + errno = ENOSYS; >>> + return -1; >>> +} >> >> I'd include in this patch this hunk from patch 17: >> >> -- >8 -- >> diff --git a/stubs/meson.build b/stubs/meson.build >> index 63392f5e78..4fd4d362f9 100644 >> --- a/stubs/meson.build >> +++ b/stubs/meson.build >> @@ -89,3 +89,7 @@ if have_system or have_user >> stub_ss.add(files('hotplug-stubs.c')) >> stub_ss.add(files('sysbus.c')) >> endif >> + >> +if host_os == 'emscripten' >> + stub_ss.add(files('emscripten.c')) >> +endif >> --- > > Actually what about checking the symbol presence in meson? > Something like (untested): > > -- >8 -- > diff --git a/meson.build b/meson.build > index b18c46d16a2..33185fdf315 100644 > --- a/meson.build > +++ b/meson.build > @@ -2654,3 +2654,2 @@ config_host_data.set('CONFIG_TIMERFD', > cc.has_function('timerfd_create')) > config_host_data.set('CONFIG_GETLOADAVG', cc.has_function('getloadavg')) > -config_host_data.set('HAVE_COPY_FILE_RANGE', cc.has_function('copy_file_range')) > config_host_data.set('HAVE_GETIFADDRS', cc.has_function('getifaddrs')) > @@ -2756,2 +2755,6 @@ config_host_data.set('HAVE_UTMPX', > > +config_host_data.set('HAVE_COPY_FILE_RANGE', cc.links(gnu_source_prefix + ''' > + #include <unistd.h> > + int main(void) { return copy_file_range(-1, NULL, -1, NULL, 0, 0); }''')) > config_host_data.set('CONFIG_EVENTFD', cc.links(''' > --- Yes indeed. We should be making sure HAVE_COPY_FILE_RANGE is unset, not providing stubs. r~
Hi Philippe and Richard, thank you for the feedback. > Actually what about checking the symbol presence in meson? > Something like (untested): > > -- >8 -- > diff --git a/meson.build b/meson.build > index b18c46d16a2..33185fdf315 100644 > --- a/meson.build > +++ b/meson.build > @@ -2654,3 +2654,2 @@ config_host_data.set('CONFIG_TIMERFD', > cc.has_function('timerfd_create')) > config_host_data.set('CONFIG_GETLOADAVG', cc.has_function('getloadavg')) > -config_host_data.set('HAVE_COPY_FILE_RANGE', > cc.has_function('copy_file_range')) > config_host_data.set('HAVE_GETIFADDRS', cc.has_function('getifaddrs')) > @@ -2756,2 +2755,6 @@ config_host_data.set('HAVE_UTMPX', > > +config_host_data.set('HAVE_COPY_FILE_RANGE', cc.links(gnu_source_prefix > + ''' > + #include <unistd.h> > + int main(void) { return copy_file_range(-1, NULL, -1, NULL, 0, 0); }''')) > config_host_data.set('CONFIG_EVENTFD', cc.links(''' > --- Emscripten doesn't provide copy_file_range implementation but it declares this function in its headers. Meson correctly detects the missing implementation and unsets HAVE_COPY_FILE_RANGE. However, the stub defined in file-posix.c causes a type conflict with the declaration from Emscripten during compilation: > ../qemu/block/file-posix.c:2019:14: error: static declaration of 'copy_file_range' follows non-static declaration > 2019 | static off_t copy_file_range(int in_fd, off_t *in_off, int out_fd, > | ^ > /emsdk/upstream/emscripten/cache/sysroot/include/unistd.h:207:9: note: previous declaration is here > 207 | ssize_t copy_file_range(int, off_t *, int, off_t *, size_t, unsigned); > | ^ > 1 error generated. If introducing a new stub isn't preferable, we could update the existing stub in file-posix.c to match the declaration used by Emscripten. The manpage[1] also aligns with this signature. [1] https://man7.org/linux/man-pages/man2/copy_file_range.2.html
Hi Philippe, > I'd include in this patch this hunk from patch 17: > > -- >8 -- > diff --git a/stubs/meson.build b/stubs/meson.build > index 63392f5e78..4fd4d362f9 100644 > --- a/stubs/meson.build > +++ b/stubs/meson.build > @@ -89,3 +89,7 @@ if have_system or have_user > stub_ss.add(files('hotplug-stubs.c')) > stub_ss.add(files('sysbus.c')) > endif > + > +if host_os == 'emscripten' > + stub_ss.add(files('emscripten.c')) > +endif > --- Sure, I'll move that hunk into this patch.
On 17/4/25 11:32, Kohei Tokunaga wrote: > > Hi Philippe and Richard, thank you for the feedback. > > > Actually what about checking the symbol presence in meson? > > Something like (untested): > > > > -- >8 -- > > diff --git a/meson.build b/meson.build > > index b18c46d16a2..33185fdf315 100644 > > --- a/meson.build > > +++ b/meson.build > > @@ -2654,3 +2654,2 @@ config_host_data.set('CONFIG_TIMERFD', > > cc.has_function('timerfd_create')) > > config_host_data.set('CONFIG_GETLOADAVG', > cc.has_function('getloadavg')) > > -config_host_data.set('HAVE_COPY_FILE_RANGE', > > cc.has_function('copy_file_range')) > > config_host_data.set('HAVE_GETIFADDRS', cc.has_function('getifaddrs')) > > @@ -2756,2 +2755,6 @@ config_host_data.set('HAVE_UTMPX', > > > > +config_host_data.set('HAVE_COPY_FILE_RANGE', cc.links(gnu_source_prefix > > + ''' > > + #include <unistd.h> > > + int main(void) { return copy_file_range(-1, NULL, -1, NULL, 0, > 0); }''')) > > config_host_data.set('CONFIG_EVENTFD', cc.links(''' > > --- > > Emscripten doesn't provide copy_file_range implementation but it declares > this function in its headers. Meson correctly detects the missing > implementation and unsets HAVE_COPY_FILE_RANGE. However, the stub defined in > file-posix.c causes a type conflict with the declaration from Emscripten > during compilation: > > > ../qemu/block/file-posix.c:2019:14: error: static declaration of > 'copy_file_range' follows non-static declaration > > 2019 | static off_t copy_file_range(int in_fd, off_t *in_off, int > out_fd, > > | ^ > > /emsdk/upstream/emscripten/cache/sysroot/include/unistd.h:207:9: > note: previous declaration is here > > 207 | ssize_t copy_file_range(int, off_t *, int, off_t *, size_t, > unsigned); > > | ^ > > 1 error generated. > > If introducing a new stub isn't preferable, we could update the existing > stub in file-posix.c to match the declaration used by Emscripten. The > manpage[1] also aligns with this signature. You are correct we should use ssize_t instead of off_t. If meson fails to link, it won't define HAVE_COPY_FILE_RANGE, so you shouldn't get "static declaration of 'copy_file_range' follows non-static declaration" right? > > [1] https://man7.org/linux/man-pages/man2/copy_file_range.2.html > <https://man7.org/linux/man-pages/man2/copy_file_range.2.html> >
Hi Philippe, > If meson fails to link, it won't define HAVE_COPY_FILE_RANGE, Yes, meson correctly detects the link failure when checking for copy_file_range, as shown in meson-log.txt: > wasm-ld: error: /tmp/emscripten_temp_oqvz296m/testfile_0.o: undefined symbol: copy_file_range and reflects this in the configure output: > Checking for function "copy_file_range" : NO > so you > shouldn't get "static declaration of 'copy_file_range' follows > non-static declaration" right? To fix this error, I needed to update the stub implementation in file-posix.c to exactly match the declaration in Emscripten's headers. Specifically, I changed the return type from off_t to ssize_t and removed the "static" qualifier. With this change, file-posix.c builds correctly under Emscripten, and there is no need to add a new stub in stubs/emscripten.c The following is the patch updates file-posix.c to solve this error: #ifndef HAVE_COPY_FILE_RANGE -static off_t copy_file_range(int in_fd, off_t *in_off, int out_fd, - off_t *out_off, size_t len, unsigned int flags) +ssize_t copy_file_range(int in_fd, off_t *in_off, int out_fd, + off_t *out_off, size_t len, unsigned int flags) { #ifdef __NR_copy_file_range return syscall(__NR_copy_file_range, in_fd, in_off, out_fd,
On 18/4/25 08:53, Kohei Tokunaga wrote: > The following is the patch updates file-posix.c to solve this error: > > #ifndef HAVE_COPY_FILE_RANGE > -static off_t copy_file_range(int in_fd, off_t *in_off, int out_fd, > - off_t *out_off, size_t len, unsigned int > flags) > +ssize_t copy_file_range(int in_fd, off_t *in_off, int out_fd, > + off_t *out_off, size_t len, unsigned int > flags) > { > #ifdef __NR_copy_file_range > return syscall(__NR_copy_file_range, in_fd, in_off, out_fd, > Yes, LGTM!
diff --git a/block/file-posix.c b/block/file-posix.c index 56d1972d15..22e0ed5069 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -110,6 +110,10 @@ #include <sys/diskslice.h> #endif +#ifdef EMSCRIPTEN +#include <sys/ioctl.h> +#endif + /* OS X does not have O_DSYNC */ #ifndef O_DSYNC #ifdef O_SYNC @@ -2010,6 +2014,7 @@ static int handle_aiocb_write_zeroes_unmap(void *opaque) return handle_aiocb_write_zeroes(aiocb); } +#ifndef EMSCRIPTEN #ifndef HAVE_COPY_FILE_RANGE static off_t copy_file_range(int in_fd, off_t *in_off, int out_fd, off_t *out_off, size_t len, unsigned int flags) @@ -2023,6 +2028,7 @@ static off_t copy_file_range(int in_fd, off_t *in_off, int out_fd, #endif } #endif +#endif /* * parse_zone - Fill a zone descriptor diff --git a/stubs/emscripten.c b/stubs/emscripten.c new file mode 100644 index 0000000000..2157d6349b --- /dev/null +++ b/stubs/emscripten.c @@ -0,0 +1,13 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +#include "qemu/osdep.h" +/* + * emscripten exposes copy_file_range declaration but doesn't provide the + * implementation in the final link. Define the stub here but avoid type + * conflict with the emscripten's header. + */ +ssize_t copy_file_range(int in_fd, off_t *in_off, int out_fd, + off_t *out_off, size_t len, unsigned int flags) +{ + errno = ENOSYS; + return -1; +}
emscripten exposes copy_file_range declaration but doesn't provide the implementation in the final link. Define the emscripten-specific stub function to avoid type conflict with the emscripten's header. Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com> --- block/file-posix.c | 6 ++++++ stubs/emscripten.c | 13 +++++++++++++ 2 files changed, 19 insertions(+) create mode 100644 stubs/emscripten.c