Re: bootstrap: improve gnulib git update logic
Am 18.08.2020 um 04:09 schrieb "Kai Torben Ohlhus: > When GNULIB_SRCDIR is set, this addition should probably work as well. > Looking at it, maybe it can be fixed by extending your code section [2] by > "git fetch": > > if test -d "$GNULIB_SRCDIR"/.git && test -n "$GNULIB_REVISION" \ > && ! git_modules_config submodule.gnulib.url >/dev/null; then > (cd "$GNULIB_SRCDIR" && git fetch && git checkout > "$GNULIB_REVISION") || cleanup_gnulib > fi I didn't receive the original message. Thanks, Kai, for the pointers. What about only fetching from the remote repository if the selected revision doesn't exist locally? diff --git "a/build-aux/bootstrap" "b/build-aux/bootstrap" index 8f76d6962..6dfbd907d 100644 --- "a/build-aux/bootstrap" +++ "b/build-aux/bootstrap" @@ -704,6 +704,8 @@ if $use_gnulib; then if test -d "$GNULIB_SRCDIR"/.git && test -n "$GNULIB_REVISION" \ && ! git_modules_config submodule.gnulib.url >/dev/null; then +(cd "$GNULIB_SRCDIR" && ! git cat-file commit "$GNULIB_REVISION" \ + && git fetch) (cd "$GNULIB_SRCDIR" && git checkout "$GNULIB_REVISION") || cleanup_gnulib fi Markus
conflicting types in windows-spawn
I tried to use the windows-spawn module in a project that defines UNICODE. Compilation of gnulib failed with the following error: libtool: compile: x86_64-w64-mingw32-gcc -DHAVE_CONFIG_H -I. -I/home/osboxes/Octave/mxe-octave/tmp-default-octave/octave-7.0.0/libgnu -I.. -I/home/osboxes/Octave/mxe-octave/usr/x86_64-w64-mingw32/include -fvisibility=hidden -g -O2 -MT windows-spawn.lo -MD -MP -MF .deps/windows-spawn.Tpo -c /home/osboxes/Octave/mxe-octave/tmp-default-octave/octave-7.0.0/libgnu/windows-spawn.c -DDLL_EXPORT -DPIC -o .libs/windows-spawn.o /home/osboxes/Octave/mxe-octave/tmp-default-octave/octave-7.0.0/libgnu/windows-spawn.c:415:1: error: conflicting types for 'compose_handles_block' 415 | compose_handles_block (const struct inheritable_handles *inh_handles, | ^ In file included from /home/osboxes/Octave/mxe-octave/tmp-default-octave/octave-7.0.0/libgnu/windows-spawn.c:21: /home/osboxes/Octave/mxe-octave/tmp-default-octave/octave-7.0.0/libgnu/windows-spawn.h:118:12: note: previous declaration of 'compose_handles_block' was here 118 | extern int compose_handles_block (const struct inheritable_handles *inh_handles, |^ make[5]: *** [Makefile:3299: windows-spawn.lo] Error 1 The following change fixes it for me: * lib/windows-spawn.h: Use ANSI structure STARTUPINFOA. --- lib/windows-spawn.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/windows-spawn.h b/lib/windows-spawn.h index 1c29d1b17..78f11893c 100644 --- a/lib/windows-spawn.h +++ b/lib/windows-spawn.h @@ -25,6 +25,9 @@ #define WIN32_LEAN_AND_MEAN #include +/* Don't assume that UNICODE is not defined. */ +#undef STARTUPINFO +#define STARTUPINFO STARTUPINFOA /* Prepares an argument vector before calling spawn(). Markus
Re: conflicting types in windows-spawn
Am 06. Mai 2021 um 14:28 Uhr schrieb "Markus Mützel": > I tried to use the windows-spawn module in a project that defines UNICODE. > > Compilation of gnulib failed with the following error: > > libtool: compile: x86_64-w64-mingw32-gcc -DHAVE_CONFIG_H -I. > -I/home/osboxes/Octave/mxe-octave/tmp-default-octave/octave-7.0.0/libgnu -I.. > -I/home/osboxes/Octave/mxe-octave/usr/x86_64-w64-mingw32/include > -fvisibility=hidden -g -O2 -MT windows-spawn.lo -MD -MP -MF > .deps/windows-spawn.Tpo -c > /home/osboxes/Octave/mxe-octave/tmp-default-octave/octave-7.0.0/libgnu/windows-spawn.c > -DDLL_EXPORT -DPIC -o .libs/windows-spawn.o > /home/osboxes/Octave/mxe-octave/tmp-default-octave/octave-7.0.0/libgnu/windows-spawn.c:415:1: > error: conflicting types for 'compose_handles_block' > 415 | compose_handles_block (const struct inheritable_handles *inh_handles, > | ^ > In file included from > /home/osboxes/Octave/mxe-octave/tmp-default-octave/octave-7.0.0/libgnu/windows-spawn.c:21: > /home/osboxes/Octave/mxe-octave/tmp-default-octave/octave-7.0.0/libgnu/windows-spawn.h:118:12: > note: previous declaration of 'compose_handles_block' was here > 118 | extern int compose_handles_block (const struct inheritable_handles > *inh_handles, > |^ > make[5]: *** [Makefile:3299: windows-spawn.lo] Error 1 > > > The following change fixes it for me: > > * lib/windows-spawn.h: Use ANSI structure STARTUPINFOA. > --- > lib/windows-spawn.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/lib/windows-spawn.h b/lib/windows-spawn.h > index 1c29d1b17..78f11893c 100644 > --- a/lib/windows-spawn.h > +++ b/lib/windows-spawn.h > @@ -25,6 +25,9 @@ > #define WIN32_LEAN_AND_MEAN > #include > > +/* Don't assume that UNICODE is not defined. */ > +#undef STARTUPINFO > +#define STARTUPINFO STARTUPINFOA > > /* Prepares an argument vector before calling spawn(). > On second thought, the header should probably better not re-define STARTUPINFO. The following alternative change might be saver: >From 84df3e82f88799d211bd4f5147473b238937d458 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=BCtzel?= Date: Thu, 6 May 2021 15:20:30 +0200 Subject: [PATCH] windows-spawn: Don't assume that UNICODE is not defined * lib/windows-spawn.h: Use ANSI structure STARTUPINFOA. --- lib/windows-spawn.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/windows-spawn.h b/lib/windows-spawn.h index 1c29d1b17..9bcfb1c82 100644 --- a/lib/windows-spawn.h +++ b/lib/windows-spawn.h @@ -116,7 +116,7 @@ extern int init_inheritable_handles (struct inheritable_handles *inh_handles, Returns 0 upon success. In case of failure, -1 is returned, with errno set. */ extern int compose_handles_block (const struct inheritable_handles *inh_handles, - STARTUPINFO *sinfo); + STARTUPINFOA *sinfo); /* Frees the memory held by a set of inheritable handles. */ extern void free_inheritable_handles (struct inheritable_handles *inh_handles); -- 2.31.1.windows.1
Re: conflicting types in windows-spawn
Am 14. Mai 2021 um 12:39 Uhr schrieb "Bruno Haible": > Yes, I agree. That's the proper way to fix it. I applied and pushed your > change. > Thanks! Thanks! Very much appreciated. Markus
Modification of environment variables on Windows
Dear gnulib developers,
We recently updated gnulib in GNU Octave to a newer revision
(d4ec02b3cc70cddaaa5183cc5a45814e0afb2292). (Kudos on the impressive speedup to
the bootstrap process.)
Since then, we are seeing warnings like the following when building for MinGW:
../../libgnu/tzset.c: In function 'rpl_tzset':
../../libgnu/tzset.c:68:24: warning: initialization of 'char *' from
incompatible pointer type 'char **' [-Wincompatible-pointer-types]
68 | for (char *s = env; *s != NULL; s++)
|^~~
../../libgnu/tzset.c:68:32: warning: comparison between pointer and integer
68 | for (char *s = env; *s != NULL; s++)
|^~
../../libgnu/tzset.c:72:28: warning: initialization of 'wchar_t *' {aka 'short
unsigned int *'} from incompatible pointer type 'wchar_t **' {aka 'short
unsigned int **'} [-Wincompatible-pointer-types]
72 | for (wchar_t *ws = wenv; *ws != NULL; ws++)
|^~~~
../../libgnu/tzset.c:72:38: warning: comparison between pointer and integer
72 | for (wchar_t *ws = wenv; *ws != NULL; ws++)
^~
IIUC, these warnings might be legitimate.
The attached patch avoids those warnings.
I'm hoping this is the correct format to propose changes to gnulib.
Best,
Markus Mützel
From 87b83c757f7d249f983410e85d13ba450d57b417 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Markus=20M=C3=BCtzel?=
Date: Sat, 27 Apr 2024 13:44:29 +0200
Subject: [PATCH] ctime, localtime, tzset, wcsftime: Check content of
environment.
* lib/ctime.c (rpl_ctime): Index into content of each environment variable.
* lib/localtime.c (rpl_localtime): Likewise.
* lib/tzset.c (rpl_tzset): Likewise.
* lib/wcsftime.c (rpl_wcsftime): Likewise.
---
lib/ctime.c | 12 ++--
lib/localtime.c | 12 ++--
lib/tzset.c | 12 ++--
lib/wcsftime.c | 12 ++--
4 files changed, 24 insertions(+), 24 deletions(-)
diff --git a/lib/ctime.c b/lib/ctime.c
index 8c54ef463c..e30b7997c8 100644
--- a/lib/ctime.c
+++ b/lib/ctime.c
@@ -63,13 +63,13 @@ rpl_ctime (const time_t *tp)
char **env = _environ;
wchar_t **wenv = _wenviron;
if (env != NULL)
-for (char *s = env; *s != NULL; s++)
- if (s[0] == 'T' && s[1] == 'Z' && s[2] == '=')
-s[0] = '$';
+for (char **s = env; *s != NULL; s++)
+ if (*s[0] == 'T' && *s[1] == 'Z' && *s[2] == '=')
+*s[0] = '$';
if (wenv != NULL)
-for (wchar_t *ws = wenv; *ws != NULL; ws++)
- if (ws[0] == L'T' && ws[1] == L'Z' && ws[2] == L'=')
-ws[0] = L'$';
+for (wchar_t **ws = wenv; *ws != NULL; ws++)
+ if (*ws[0] == L'T' && *ws[1] == L'Z' && *ws[2] == L'=')
+*ws[0] = L'$';
}
#endif
diff --git a/lib/localtime.c b/lib/localtime.c
index f0e91ac647..1c6bb9856e 100644
--- a/lib/localtime.c
+++ b/lib/localtime.c
@@ -63,13 +63,13 @@ rpl_localtime (const time_t *tp)
char **env = _environ;
wchar_t **wenv = _wenviron;
if (env != NULL)
-for (char *s = env; *s != NULL; s++)
- if (s[0] == 'T' && s[1] == 'Z' && s[2] == '=')
-s[0] = '$';
+for (char **s = env; *s != NULL; s++)
+ if (*s[0] == 'T' && *s[1] == 'Z' && *s[2] == '=')
+*s[0] = '$';
if (wenv != NULL)
-for (wchar_t *ws = wenv; *ws != NULL; ws++)
- if (ws[0] == L'T' && ws[1] == L'Z' && ws[2] == L'=')
-ws[0] = L'$';
+for (wchar_t **ws = wenv; *ws != NULL; ws++)
+ if (*ws[0] == L'T' && *ws[1] == L'Z' && *ws[2] == L'=')
+*ws[0] = L'$';
}
#endif
diff --git a/lib/tzset.c b/lib/tzset.c
index f307f0c3d1..c8d48e1afb 100644
--- a/lib/tzset.c
+++ b/lib/tzset.c
@@ -65,13 +65,13 @@ rpl_tzset (void)
char **env = _environ;
wchar_t **wenv = _wenviron;
if (env != NULL)
-for (char *s = env; *s != NULL; s++)
- if (s[0] == 'T' && s[1] == 'Z' && s[2] == '=')
-s[0] = '$';
+for (char **s = env; *s != NULL; s++)
+ if (*s[0] == 'T' && *s[1] == 'Z' && *s[2] == '=')
+*s[0] = '$';
if (wenv != NULL)
-for (wchar_t *ws = wenv; *ws != NULL; ws++)
- if (ws[0] == L'T' && ws[1] == L&
Re: Modification of environment variables on Windows
Hi Bruno, > > The attached patch avoids those warnings. > > Thanks, but it does not do the right thing: *s[1] accesses the first character > of the string after s. What was meant was to access the second character of > the string at s; this needs to be written as (*s)[1]. Oof. You are absolutely right. What a difference some parenthesis make. > I'm committing this patch instead. Thank you for spotting the error in the proposed patch and pushing a fix so quickly. Markus
Fetch from existing gnulib Git repository if needed
Dear gnulib developers,
GNU Octave uses Mercurial as the VCS of its main repository.
Developers are using the bootstrap script of gnulib to automatically clone its
Git repository in a subdirectory of Octave's source tree. The revision that
we'd like to use is set in the bootstrap.conf script. Currently, that is
: ${GNULIB_REVISION=d4ec02b3cc70cddaaa5183cc5a45814e0afb2292}
This is working perfectly for a fresh clone of Octave's source tree. However,
when we update GNULIB_REVISION to a newer revision and a user/developer ran the
bootstrap script before, running the bootstrap script again fails with an error
like the following:
./bootstrap: Bootstrapping from checked-out octave sources...
fatal: reference is not a tree: d4ec02b3cc70cddaaa5183cc5a45814e0afb2292
program finished with exit code 128
To work around that, a user/developer could manually fetch from the remote
repository. That is a bit more tedious when it comes to CI installations that
usually need no manual interaction.
As a workaround we are applying the attached patch to the bootstrap-funclib.sh
script to automatically fetch from the remote gnulib repository if the
GNULIB_REVISION isn't found in the local gnulib Git repository.
Would it be possible to make a similar change in gnulib so that updating to a
newer gnulib revision becomes a bit easier for that configuration?
Markus
Update bootstrap script from upstream gnulib to automatically fetch from
repository if needed
diff -r c51b07a71421 bootstrap-funclib.sh
--- a/bootstrap-funclib.sh Fri Apr 26 13:33:37 2024 -0400
+++ b/bootstrap-funclib.sh Fri Apr 26 20:00:21 2024 +0200
@@ -532,6 +532,10 @@
# The subdirectory 'gnulib' already exists.
if test -n "$GNULIB_REVISION"; then
if test -d "$gnulib_path/.git"; then
+if ! git --git-dir="$gnulib_path"/.git cat-file \
+ commit "$GNULIB_REVISION"; then
+ git --git-dir="$gnulib_path"/.git fetch
+fi
(cd "$gnulib_path" && git checkout "$GNULIB_REVISION") || exit 1
else
die "Error: GNULIB_REVISION is specified in bootstrap.conf," \
Re: Fetch from existing gnulib Git repository if needed
Hi Bruno,
> > As a workaround we are applying the attached patch to the
> > bootstrap-funclib.sh script to automatically fetch from the remote gnulib
> > repository if the GNULIB_REVISION isn't found in the local gnulib Git
> > repository.
>
> Thanks for the patch. But note that GNULIB_REVISION can hold either a commit
> hash or the name of a branch (such as 'stable-202401'). So, we have 4 cases:
> (a) a commit hash, already present
> (b) a commit hash, not known
> (c) a branch, already present
> (d) a branch, not known
>
> The command 'git cat-file commit $GNULIB_REVISION' returns true in the cases
> (a) and (c). So, your patch would trigger a 'git fetch' in the cases (b) and
> (d). But in case (d), the 'git fetch' is useless:
> 'git cat-file commit $GNULIB_REVISION' would still fail afterwards.
>
> One can distinguish the four cases in more detail using the commands
> git rev-list --quiet $GNULIB_REVISION --
> which tests for case (a) and
> git show-ref --verify --quiet refs/heads/$GNULIB_REVISION
> which tests for case (c). This would allow us to do 'git fetch' only in case
> (b).
>
> However, I believe the patch below is simpler and achieves the same goal.
Thank you for looking into this. However, it looks like $GNULIB_SRCDIR is empty
for us. So, the change doesn't seem to make a difference. Executing bootstrap
after a revision bump still fails if the bootstrap script was already run
before.
I like your idea of fetching if the checkout failed though.
The attached diff seems to be working for our use case. Would it be possible to
apply something like that in gnulib?
Markus
diff -r 0fbf06e4b460 bootstrap-funclib.sh
--- a/bootstrap-funclib.sh Sat Apr 27 17:33:00 2024 +0200
+++ b/bootstrap-funclib.sh Sun Apr 28 11:04:52 2024 +0200
@@ -462,7 +462,17 @@
|| die "Error: --gnulib-srcdir or \$GNULIB_SRCDIR is specified," \
"but does not contain gnulib-tool"
if test -n "$GNULIB_REVISION" && $use_git; then
- (cd "$GNULIB_SRCDIR" && git checkout "$GNULIB_REVISION") || exit $?
+ # The 'git checkout "$GNULIB_REVISION"' command succeeds if the
+ # GNULIB_REVISION is a commit hash that exists locally, or if it is
+ # branch name that can be fetched from origin. It fails, however,
+ # if the GNULIB_REVISION is a commit hash that only exists in origin.
+ # In this case, we need a 'git fetch' and then retry
+ # 'git checkout "$GNULIB_REVISION"'.
+ (cd "$GNULIB_SRCDIR" \
+&& { git checkout "$GNULIB_REVISION" 2>/dev/null \
+ || { git fetch origin && git checkout "$GNULIB_REVISION"; }
+}
+ ) || exit $?
fi
else
if ! $use_git; then
@@ -532,7 +542,17 @@
# The subdirectory 'gnulib' already exists.
if test -n "$GNULIB_REVISION"; then
if test -d "$gnulib_path/.git"; then
-(cd "$gnulib_path" && git checkout "$GNULIB_REVISION") || exit 1
+# The 'git checkout "$GNULIB_REVISION"' command succeeds if the
+# GNULIB_REVISION is a commit hash that exists locally, or if it is
+# branch name that can be fetched from origin. It fails, however,
+# if the GNULIB_REVISION is a commit hash that only exists in
origin.
+# In this case, we need a 'git fetch' and then retry
+# 'git checkout "$GNULIB_REVISION"'.
+(cd "$gnulib_path" \
+ && { git checkout "$GNULIB_REVISION" 2>/dev/null \
+ || { git fetch origin && git checkout "$GNULIB_REVISION"; }
+ }
+) || exit $?
else
die "Error: GNULIB_REVISION is specified in bootstrap.conf," \
"but '$gnulib_path' contains no git history"
Re: Fetch from existing gnulib Git repository if needed
Hi Bruno, Bruno Haible wrote: > Markus Mützel wrote: > > However, it looks like $GNULIB_SRCDIR is empty for us. So, the change > > doesn't seem to make a difference. Executing bootstrap after a revision > > bump still fails if the bootstrap script was already run before. > > Oh, there were two 'git checkout' commands and I enhanced only one of them... > > Should now be fixed, like this. Thanks for the feedback! Thank you for the quick fix. It seems to automatically fetch if needed for us now. Markus
Unknown type name 'wint_t' when targeting Cygwin
Dear gnulib maintainers, We recently updated gnulib to a newer revision in GNU Octave (currently 92d80242ad1344b5364ca9bd1d995d68c3a73ef7). Since then we are seeing compilation errors like the following when targeting Cygwin: In file included from /usr/include/sys/reent.h:16, from /usr/include/wchar.h:6, from ./wchar.h:80, from /usr/include/uchar.h:5, from ./uchar.h:45, from ../../libgnu/btoc32.c:23: /usr/include/sys/_types.h:167:5: error: unknown type name 'wint_t' 167 | wint_t __wch; | ^~ Is this because something is missing in gnulib? Are we doing something wrong? I can try to provide logs and more information if needed. Markus
Re: Unknown type name 'wint_t' when targeting Cygwin
Hi Bruno, Bruno Haible wrote > Which Cygwin version, please? That error occurred, e.g., in a CI run https://github.com/gnu-octave/octave/actions/runs/8873331621/job/24358996111 The log of that run contains the following line: Starting cygwin install, version 2.932 Is that the Cygwin version? All packages should be the latest stable versions that are distributed by the Cygwin project. > Also, what is the gcc command line of this particular compilation unit? > (`make V=1`) The CI uses a parallel make. IIUC, the following lines in the log match and reproduce one of the errors: /bin/sh ../libtool --tag=CC --mode=compile gcc -DHAVE_CONFIG_H -I. -I../../libgnu -I.. -Wno-cast-qual -Wno-conversion -Wno-float-equal -Wno-sign-compare -Wno-undef -Wno-unused-function -Wno-unused-parameter -Wno-float-conversion -Wimplicit-fallthrough -Wno-pedantic -Wno-sign-conversion -Wno-type-limits -Wno-unsuffixed-float-constants -g -O2 -MT libgnu_la-areadlink.lo -MD -MP -MF .deps/libgnu_la-areadlink.Tpo -c -o libgnu_la-areadlink.lo `test -f 'areadlink.c' || echo '../../libgnu/'`areadlink.c libtool: compile: gcc -DHAVE_CONFIG_H -I. -I../../libgnu -I.. -Wno-cast-qual -Wno-conversion -Wno-float-equal -Wno-sign-compare -Wno-undef -Wno-unused-function -Wno-unused-parameter -Wno-float-conversion -Wimplicit-fallthrough -Wno-pedantic -Wno-sign-conversion -Wno-type-limits -Wno-unsuffixed-float-constants -g -O2 -MT libgnu_la-areadlink.lo -MD -MP -MF .deps/libgnu_la-areadlink.Tpo -c ../../libgnu/areadlink.c -DDLL_EXPORT -DPIC -o .libs/libgnu_la-areadlink.o In file included from /usr/include/sys/reent.h:16, from /usr/include/stdlib.h:18, from ./stdlib.h:36, from ../../libgnu/areadlink.h:21, from ../../libgnu/areadlink.c:25: /usr/include/sys/_types.h:167:5: error: unknown type name 'wint_t' 167 | wint_t __wch; | ^~ Please, let me know if that is the information that you are looking for or if you need further details. Markus
Re: Unknown type name 'wint_t' when targeting Cygwin
Hi Bruno, > The patch below fixes it for me. It reverts a workaround for gcc 14.0.1 > from a few days ago, for which I'm adding a different workaround later. Thank you very much. That fixed the build errors also for us. (Tested with 6213c5bd72d15ca5e1ea9c34122899e02fed448c.) Markus
fseek and ftell with large files on Windows
Octave is using gnulib's "fseek" and "ftell" functions. A user reported that they have issues when using these functions with large files on Windows: https://savannah.gnu.org/bugs/index.php?66399 If I understand the code paths for Windows correctly, the issue might be caused by the fact that the "fseek" and "ftell" functions are forwarding to "_fseeki64" or "_ftelli64", respectively, if the size of the "off_t" type is smaller than 64-bit. They are forwarding to "fseek" and "ftell" otherwise. However, even if the "off_t" type is 64-bit (or larger), "fseek" and "ftell" still return "long" on Windows which is only 32-bit wide: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/ftell-ftelli64?view=msvc-170 The following change avoids the issue described in the above bug report for me. I don't know whether that change is correct though. Thank you, Markus diff --git a/lib/fseeko.c b/lib/fseeko.c index 2c3b053a3b..30f8f70afe 100644 --- a/lib/fseeko.c +++ b/lib/fseeko.c @@ -31,7 +31,7 @@ fseeko (FILE *fp, off_t offset, int whence) # undef fseek # define fseeko fseek #endif -#if _GL_WINDOWS_64_BIT_OFF_T +#if _WIN32 # undef fseeko # if HAVE__FSEEKI64 && HAVE_DECL__FSEEKI64 /* msvc, mingw since msvcrt8.0, mingw64 */ # define fseeko _fseeki64 diff --git a/lib/ftello.c b/lib/ftello.c index 88247bca8e..2f83c61c0e 100644 --- a/lib/ftello.c +++ b/lib/ftello.c @@ -34,7 +34,7 @@ ftello (FILE *fp) # undef ftell # define ftello ftell #endif -#if _GL_WINDOWS_64_BIT_OFF_T +#if _WIN32 # undef ftello # if HAVE__FTELLI64 /* msvc, mingw64 */ # define ftello _ftelli64
Re: fseek and ftell with large files on Windows
Am Montag, 11. November 2024 um 19:50 schrieb "Bruno Haible": > Hi, > > Markus Mützel wrote: > > Octave is using gnulib's "fseek" and "ftell" functions. A user reported that they have issues when using these functions with large files on Windows: > > https://savannah.gnu.org/bugs/index.php?66399 > > This is explained in the Gnulib documentation: > https://www.gnu.org/software/gnulib/manual/html_node/fseek.html > https://www.gnu.org/software/gnulib/manual/html_node/ftell.html > Thank you for the clarification. And sorry for being slightly confused when I wrote the initial email: The Octave functions that are being called in the reproducer are named "fseek" and "ftell". But the gnulib functions that are called internally are "fseeko" and "ftello". If I understood Paul Eggert in that bug report correctly, he found at least one place where Octave is using "fseek" instead of "fseeko". But stepping through with a debugger, - at least in the code paths that reproduce the issue - "fseeko" and "ftello" are used as far as I can tell. > > If I understand the code paths for Windows correctly > > It's not specific to Windows. You get the same effects on all 32-bit platforms > as well. > > > diff --git a/lib/fseeko.c b/lib/fseeko.c > > index 2c3b053a3b..30f8f70afe 100644 > > --- a/lib/fseeko.c > > +++ b/lib/fseeko.c > > @@ -31,7 +31,7 @@ fseeko (FILE *fp, off_t offset, int whence) > > # undef fseek > > # define fseeko fseek > > #endif > > -#if _GL_WINDOWS_64_BIT_OFF_T > > +#if _WIN32 > > # undef fseeko > > # if HAVE__FSEEKI64 && HAVE_DECL__FSEEKI64 /* msvc, mingw since msvcrt8.0, mingw64 */ > > # define fseeko _fseeki64 > > diff --git a/lib/ftello.c b/lib/ftello.c > > index 88247bca8e..2f83c61c0e 100644 > > --- a/lib/ftello.c > > +++ b/lib/ftello.c > > @@ -34,7 +34,7 @@ ftello (FILE *fp) > > # undef ftell > > # define ftello ftell > > #endif > > -#if _GL_WINDOWS_64_BIT_OFF_T > > +#if _WIN32 > > # undef ftello > > # if HAVE__FTELLI64 /* msvc, mingw64 */ > > # define ftello _ftelli64 > > This patch cannot help, since the problem is with using a 'long' as offset. Gnulib > can't change that, since the prototype of fseek and ftell is specified by ISO C. Octave is using 'off_t' as offset when calling "fseeko". Checking the content of the generated "sys/types.h" header, I see the following: /* Override off_t if Large File Support is requested on native Windows. */ #if 0 /* Same as int64_t in . */ # if defined _MSC_VER # define off_t __int64 # else # define off_t long long int # endif /* Indicator, for gnulib internal purposes. */ # define _GL_WINDOWS_64_BIT_OFF_T 1 #endif If I understand correctly, the fact that this section is skipped means that 'off_t' has a 64-bit size here (on mingw-w64) and doesn't need to be redefined. Looping back to the previously proposed changes for "fseeko.c" and "fseeko.c": In the current version 'fseeko' is defined to 'fseek' for the function body. But 'fseek' uses an offset that is of type 'long' on Windows. So, it won't work with large files. With the proposed changes, 'fseeko' would be defined to '_fseeki64' for the function body. (And similar changes for "ftello.c".) Isn't that needed for large files on Windows? Markus
Re: fseek and ftell with large files on Windows
My email provider started to mess with outgoing only-text messages. So, apologies if some characters are replaced incorrectly. Am 13. November 2024 um 00:54 schrieb "Bruno Haible": > In order to make sure that Octave does not have bugs with files > 2 GiB > (without restrictions like "as far as I can tell"), you need four things: > 1) Make sure to request the modules 'fseeko', 'ftello' from gnulib, not > 'fseek' and 'ftell'. > 2) Use "nm octave" (or "nm octave.exe" on mingw), as well as > "nm liboctave.so", "nm liboctgui.so", "nm liboctinterp.so", > to verify that the binaries don't use the functions 'fseek', 'ftell'. > 3) In the code around the fseeko and ftello calls, drop the use of the > 'long' type and use 'off_t' instead. > 4) Use gnulib module 'largefile' as well. Thank you for the tips. I made some local changes to eliminate remaining uses of "fseek" in Octave. (None of them were relevant for the reported issue.) With those, liboctave.dll still used "ftell". It took me a while to assure it isn't coming from anywhere else. But the only place that made use of that symbol is from "gnulib/lib/ftello.c". I found the following in the generated "config.in.h": /* Define to 1 if fseeko (and presumably ftello) exists and is declared. */ #undef HAVE_FSEEKO That was replaced with the following in the generated "config.h" after running the configure script: /* Define to 1 if fseeko (and presumably ftello) exists and is declared. */ #define HAVE_FSEEKO 1 But no match for HAVE_FTELLO in "config.in.h" nor in "config.h". With the following change in gnulib, liboctave.dll no longer uses "ftell": diff --git a/lib/ftello.c b/lib/ftello.c index 88247bca8e..29158b354d 100644 --- a/lib/ftello.c +++ b/lib/ftello.c @@ -30,7 +30,7 @@ off_t ftello (FILE *fp) #undef ftello -#if !HAVE_FTELLO +#if !HAVE_FSEEKO # undef ftell # define ftello ftell #endif Does that look more reasonable? Markus
Re: 'daylight' redeclared as different kind of symbol with mingw-w64
The following might be relevant:
configure:79124: checking spelling of daylight variable
configure:79153: gcc -o conftest.exe -g -ggdb -O0 -pthread -fopenmp
-fexceptions conftest.c -lpthread -lm >&5
conftest.c: In function 'main':
conftest.c:341:8: error: returning 'int * (*)(void)' from a function with
return type 'int' makes integer from pointer without a cast [-Wint-conversion]
341 | return __daylight;
|^~
configure:79153: $? = 1
configure: failed program was:
| #include
|
| #ifdef F77_DUMMY_MAIN
|
| # ifdef __cplusplus
| extern "C"
| # endif
|int F77_DUMMY_MAIN() { return 1; }
|
| #endif
| int
| main (void)
| {
| return __daylight;
| ;
| return 0;
| }
|
That is with GCC 14.2.0 if that matters.
Markus
'daylight' redeclared as different kind of symbol with mingw-w64
Hello, I tried to build Octave for Windows after updating gnulib from 6213c5bd72d15ca5e1ea9c34122899e02fed448c to f3d96537413f1fc0913cd290b1eea35386295b1b. I'm using mingw-w64 at commit cf5c50cce0b4f8610eafd95443d1d3767de386a5. Some configure tests are failing now that have been passing previously with errors like the following in config.log: configure:91729: checking for libqhull_r/libqhull_r.h configure:91729: gcc -c -g -ggdb -O0 -pthread -fopenmp -fexceptions conftest.c >&5 In file included from C:/msys64/mingw64/include/libqhull_r/user_r.h:33, from C:/msys64/mingw64/include/libqhull_r/libqhull_r.h:29, from conftest.c:503: C:/msys64/mingw64/include/time.h:274:22: error: 'daylight' redeclared as different kind of symbol 274 | _CRTIMP extern int daylight __MINGW_ATTRIB_DEPRECATED_UCRT; | ^~~~ conftest.c:330:20: note: previous declaration of 'daylight' with type 'int *(void)' 330 | #define __daylight daylight |^~~~ If I try to run make anyway, it fails with a similar error: In file included from ./time.h:54, from ./sys/stat.h:51, from ./fcntl.h:65, from ../../libgnu/access.c:23: C:/msys64/mingw64/include/time.h:274:22: error: 'daylight' redeclared as different kind of symbol 274 | _CRTIMP extern int daylight __MINGW_ATTRIB_DEPRECATED_UCRT; | ^~~~ In file included from ../../libgnu/access.c:17: ../config.h:3195:20: note: previous declaration of 'daylight' with type 'int *(void)' 3195 | #define __daylight daylight |^~~~ Am I doing something wrong? Is that an issue in mingw-w64? Can gnulib do something about it? Markus
Re: 'daylight' redeclared as different kind of symbol with mingw-w64
Am 14. November 2024 um 18:43 schrieb "Paul Eggert": > That 'daylight' business was a fiasco for other reasons, which I had > been meaning to clean up but haven't gotten around to. I installed the > attached Gnulib patch for now; please give it a try. > > As the Gnulib manual says, nobody should use 'daylight'. Thank you for the quick fix. It is building without issues for me now when targeting mingw-w64. Markus
Re: fseek and ftell with large files on Windows
Am 14. November 2024 um 13:31 schrieb "Bruno Haible": > No, this is not needed. In a testdir of the modules fseeko, ftello, fsync, > on mingw: > - we have REPLACE_FSEEKO=1, hence fseeko.c gets compiled, and it uses > the mingw fseeko function, > - the two new unit tests pass. > - Apparently the mingw fseeko already supports the 64-bit argument fine. "fseeko" has the following prototype in mingw-w64: https://sourceforge.net/p/mingw-w64/code/HEAD/tree/trunk/mingw-w64-crt/stdio/fseeko32.c int fseeko(FILE* stream, _off_t offset, int whence) The offset is of type "_off_t". That type is defined like this in mingw-w64: https://sourceforge.net/p/mingw-w64/code/HEAD/tree/trunk/mingw-w64-headers/crt/_mingw_off_t.h typedef long _off_t; "long" is a 32-bit integer on Windows. So, the "fseeko" function from mingw-w64 shouldn't work for large files. If the unit test passes with it, it might be passing erroneously. Does that make sense? Markus
Re: fseek and ftell with large files on Windows
Am 14. November 2024 um 05:18 schrieb "Bruno Haible": > And with this additional patch, ftello() works on large files on mingw. > > > 2024-11-14 Bruno Haible > >ftello: Fix override on mingw. >Reported by Markus Mützel in >. >* lib/ftello.c: Test whether module 'largefile' is in use, not >whether it had to override 'off_t'. > > diff --git a/lib/ftello.c b/lib/ftello.c > index 88247bca8e..37fe93a34e 100644 > --- a/lib/ftello.c > +++ b/lib/ftello.c > @@ -34,7 +34,7 @@ ftello (FILE *fp) > # undef ftell > # define ftello ftell > #endif > -#if _GL_WINDOWS_64_BIT_OFF_T > +#if (defined _WIN32 && !defined __CYGWIN__) && _FILE_OFFSET_BITS == 64 > # undef ftello > # if HAVE__FTELLI64 /* msvc, mingw64 */ > # define ftello _ftelli64 > Thank you very much for the patch series. With the same reasoning as for ftello.c, should this also be changed in fseeko.c? diff --git a/lib/fseeko.c b/lib/fseeko.c index 2c3b053a3b..9d003208ba 100644 --- a/lib/fseeko.c +++ b/lib/fseeko.c @@ -31,7 +31,10 @@ fseeko (FILE *fp, off_t offset, int whence) # undef fseek # define fseeko fseek #endif -#if _GL_WINDOWS_64_BIT_OFF_T +#if (defined _WIN32 && !defined __CYGWIN__) \ +/* We need to test _FILE_OFFSET_BITS for mingw-w64 */ \ +/* and _GL_WINDOWS_64_BIT_OFF_T for MSVC. */ \ +&& (_FILE_OFFSET_BITS == 64 || _GL_WINDOWS_64_BIT_OFF_T) # undef fseeko # if HAVE__FSEEKI64 && HAVE_DECL__FSEEKI64 /* msvc, mingw since msvcrt8.0, mingw64 */ # define fseeko _fseeki64
Re: fseek and ftell with large files on Windows
Am 14. November 2024 um 15:31 schrieb "Bruno Haible" > > Markus Mützel wrote: > > If the unit test passes with it, it might be passing erroneously. > > No: Before making the fixes and adding the unit test, I verified that without > the fixes, the unit test failed. In other words, the unit test is able > to detect the bugs we are interested in. > > > > - Apparently the mingw fseeko already supports the 64-bit argument fine. > > > > > > "fseeko" has the following prototype in mingw-w64: > > https://sourceforge.net/p/mingw-w64/code/HEAD/tree/trunk/mingw-w64-crt/stdio/fseeko32.c > > int fseeko(FILE* stream, _off_t offset, int whence) > > > > The offset is of type "_off_t". That type is defined like this in mingw-w64: > > https://sourceforge.net/p/mingw-w64/code/HEAD/tree/trunk/mingw-w64-headers/crt/_mingw_off_t.h > > typedef long _off_t; > > > > "long" is a 32-bit integer on Windows. So, the "fseeko" function from mingw-w64 shouldn't work for large files. > > > > Does that make sense? > > No. You missed the facts that > - the Gnulib 'largefile' module defines _FILE_OFFSET_BITS to 64 on mingw, > - mingw-w64-headers/crt/stdio.h thus does a > #define fseeko fseeko64 > - fseeko64 is defined in mingw-w64-crt/stdio/fseeko64.c and takes a 64-bit > argument. > Thank you for clarifying. That makes sense. Markus
Re: Returning stack-allocated memory in gettext.h
Am 19. Juni 2025 um 20:23 schrieb "Bruno Haible": > Hi, > > Markus Mützel wrote: > > Building Octave with the CodeQL analyzer on GitHub gives the following > > alert in > > a header from gnulib: > > > > Returning stack-allocated memory > > > > libgnu/gettext.h:254 > > May return stack-allocated memory from msg_ctxt_id. > > It's a false alarm. Already answered in [1] and [2]. > > Bruno > > [1] https://lists.gnu.org/archive/html/bug-gettext/2023-07/msg3.html > [2] https://lists.gnu.org/archive/html/bug-gnulib/2023-11/msg00148.html Thank you for the quick reply and for these pointers. Just to clarify: You were writing that dcgettext will return either the second argument, or a string that has indefinite extent. If I understand correctly, the second argument runs out of scope when the function returns. Is that true? Markus
Returning stack-allocated memory in gettext.h
Hello, Building Octave with the CodeQL analyzer on GitHub gives the following alert in a header from gnulib: Returning stack-allocated memory libgnu/gettext.h:254 May return stack-allocated memory from msg_ctxt_id. If I'm reading that file correctly, "translation" got assigned with "msg_ctxt_id". The latter is either a VLA which is out of scope when the function returns, or it was freed. Either way, the returned pointer might be invalid. I might very well miss something though. Is this actually an issue? Or is it a false positive? Thank you for your help. Markus
Re: Returning stack-allocated memory in gettext.h
Am 19. Juni 2025 um 20:51 schrieb "Bruno Haible": > Markus Mützel wrote: > > > [1] https://lists.gnu.org/archive/html/bug-gettext/2023-07/msg3.html > > > [2] https://lists.gnu.org/archive/html/bug-gnulib/2023-11/msg00148.html > > > > > > Thank you for the quick reply and for these pointers. > > > > Just to clarify: You were writing that dcgettext will return either the > > second > > argument, or a string that has indefinite extent. > > Yes. > > > If I understand correctly, the second argument runs out of scope when the > > function returns. Is that true? > > Yes, but that is irrelevant since that second argument is not the return > value. In the case that dcgettext returns the second argument, it would be assigned to "translation". Wouldn't that mean that a pointer to an "object" that would be out of scope would be returned from the function in that case? Markus
