Problems with 82e79c63642c, NOT_CONSTANT with sigfillset()

Problems with 82e79c63642c, NOT_CONSTANT with sigfillset()

[Torsten Bögershausen]

There is a problem with the somewhat new
commit 82e79c63642c introducing NOT_CONSTANT with sigfillset():

Compiling on older (?) MacOs machine leads to this:
     Undefined symbols for architecture x86_64:
       "_false_but_the_compiler_does_not_know_it_", referenced from:
           _start_command in libgit.a(run-command.o)

What is the best thing to avoid this ?

The introduced "false_but_the_compiler_does_not_know_it" triggers here
because there is a proper prototype in signal.h and then a
#define sigfillset() ...
further down in the file, which can never fail.

a) Define a sigfillset_or_die_if_needed macro,
   which does what we have today, and is simply defined
   as sigfillset() without any error checking for MacOs(some version)
b) Revert the commit (and fix the conflicts)
c) Anything better ?

I have a raw patch for b), not fully cooked, as the commit message
is the hardest part. Before sending that out:
Are the thoughts about a better solution ?

Re: Problems with 82e79c63642c, NOT_CONSTANT with sigfillset()

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> There is a problem with the somewhat new
> commit 82e79c63642c introducing NOT_CONSTANT with sigfillset():
>
> Compiling on older (?) MacOs machine leads to this:
>     Undefined symbols for architecture x86_64:
>       "_false_but_the_compiler_does_not_know_it_", referenced from:
>           _start_command in libgit.a(run-command.o)
>
> What is the best thing to avoid this ?
>
> The introduced "false_but_the_compiler_does_not_know_it" triggers here
> because there is a proper prototype in signal.h and then a
> #define sigfillset() ...
> further down in the file, which can never fail.
>
> a) Define a sigfillset_or_die_if_needed macro,
>   which does what we have today, and is simply defined
>   as sigfillset() without any error checking for MacOs(some version)
> b) Revert the commit (and fix the conflicts)
> c) Anything better ?
>
> I have a raw patch for b), not fully cooked, as the commit message
> is the hardest part. Before sending that out:
> Are the thoughts about a better solution ?

How about figuring out why it does not work on your system?  

Namely, compiling compiler-tricks/not-constant.c should yield
compiler-tricks/not-constant.o and that should be "ar"ed into
libgit.a, just like config.o, connect.o, etc., so I cannot see how
your ld fails to find false_but_the_compiler_does_not_know_it when
it has no problem with say git_config or other global symbols.

Re: Problems with 82e79c63642c, NOT_CONSTANT with sigfillset()

[Koji Nakamaru]

On Tue, May 6, 2025 at 5:47 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Torsten Bögershausen <tboegi@web.de> writes:
>
> > There is a problem with the somewhat new
> > commit 82e79c63642c introducing NOT_CONSTANT with sigfillset():
> >
> > Compiling on older (?) MacOs machine leads to this:
> >     Undefined symbols for architecture x86_64:
> >       "_false_but_the_compiler_does_not_know_it_", referenced from:
> >           _start_command in libgit.a(run-command.o)
> >
> > What is the best thing to avoid this ?
> >
> > The introduced "false_but_the_compiler_does_not_know_it" triggers here
> > because there is a proper prototype in signal.h and then a
> > #define sigfillset() ...
> > further down in the file, which can never fail.
> >
> > a) Define a sigfillset_or_die_if_needed macro,
> >   which does what we have today, and is simply defined
> >   as sigfillset() without any error checking for MacOs(some version)
> > b) Revert the commit (and fix the conflicts)
> > c) Anything better ?
> >
> > I have a raw patch for b), not fully cooked, as the commit message
> > is the hardest part. Before sending that out:
> > Are the thoughts about a better solution ?
>
> How about figuring out why it does not work on your system?
>
> Namely, compiling compiler-tricks/not-constant.c should yield
> compiler-tricks/not-constant.o and that should be "ar"ed into
> libgit.a, just like config.o, connect.o, etc., so I cannot see how
> your ld fails to find false_but_the_compiler_does_not_know_it when
> it has no problem with say git_config or other global symbols.
>

I reproduced the issue with Xcode 14.2 on macOS Monterey. It seems to
be caused by the ld command's bug and can be avoided by tweaking
Makefile:

diff --git a/Makefile b/Makefile
index 8a7f1c7654..8a06db8400 100644
--- a/Makefile
+++ b/Makefile
@@ -1387,7 +1387,7 @@ UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
 UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/lib-reftable.o

 # xdiff and reftable libs may in turn depend on what is in libgit.a
-GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(LIB_FILE)
+GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB)
$(LIB_FILE) compiler-tricks/not-constant.o
 EXTLIBS =

 GIT_USER_AGENT = git/$(GIT_VERSION)

or by explicitly initializing false_but_the_compiler_does_not_know_it_
(which changes the symbol type from 'C' to 'S'):

diff --git a/compiler-tricks/not-constant.c b/compiler-tricks/not-constant.c
index 1da3ffc2f5..9fb4f275b1 100644
--- a/compiler-tricks/not-constant.c
+++ b/compiler-tricks/not-constant.c
@@ -1,2 +1,2 @@
 #include <git-compat-util.h>
-int false_but_the_compiler_does_not_know_it_;
+int false_but_the_compiler_does_not_know_it_ = 0;

Koji Nakamaru

[PATCH/RFC v1 1/1] intialize false_but_the_compiler_does_not_know_it_

[tboegi]

From: Torsten Bögershausen <tboegi@web.de>

Compiling/linking 82e79c63642c on an older MacOs machine leads to this:
Undefined symbols for architecture x86_64:
  "_false_but_the_compiler_does_not_know_it_", referenced from:
      _start_command in libgit.a(run-command.o)

The linker doesn't seem to pick up the symbol:
"false_but_the_compiler_does_not_know_it_"

Initializing the variable to 0 fixes the problem:
The symbol type changes from 'C' to 'S' and is picked up by the linker.

Helped-by: Koji Nakamaru <koji.nakamaru@gree.net>
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 compiler-tricks/not-constant.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Koji Nakamaru: Thanks for the digging.
This patch fixes the problem here -  as a side note,
the change in Makefile alone doesn't help.


diff --git a/compiler-tricks/not-constant.c b/compiler-tricks/not-constant.c
index 1da3ffc2f5..9fb4f275b1 100644
--- a/compiler-tricks/not-constant.c
+++ b/compiler-tricks/not-constant.c
@@ -1,2 +1,2 @@
 #include <git-compat-util.h>
-int false_but_the_compiler_does_not_know_it_;
+int false_but_the_compiler_does_not_know_it_ = 0;
-- 
2.39.5

Re: [PATCH/RFC v1 1/1] intialize false_but_the_compiler_does_not_know_it_

[Junio C Hamano]

tboegi@web.de writes:

> From: Torsten Bögershausen <tboegi@web.de>
>
> Compiling/linking 82e79c63642c on an older MacOs machine leads to this:
> Undefined symbols for architecture x86_64:
>   "_false_but_the_compiler_does_not_know_it_", referenced from:
>       _start_command in libgit.a(run-command.o)
>
> The linker doesn't seem to pick up the symbol:
> "false_but_the_compiler_does_not_know_it_"
>
> Initializing the variable to 0 fixes the problem:
> The symbol type changes from 'C' to 'S' and is picked up by the linker.
>
> Helped-by: Koji Nakamaru <koji.nakamaru@gree.net>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
>  compiler-tricks/not-constant.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thanks, both.

The alleged ld bug and this fix I can sort of believe.

The resulting object file from the original code would not have any
data, but just declares a BSS symbol in common, which may be
unusual, and this forces us to have an explicit initialization
data.

Just for reference (as the proposed log message refers to an "older
macOS"), do we know if the toolchain on a more recent release of
macOS work without this workaround already?  It may be nice to tell
users what version they need to avoid the same issue in their own
program.

Will queue.  Thanks.



> Koji Nakamaru: Thanks for the digging.
> This patch fixes the problem here -  as a side note,
> the change in Makefile alone doesn't help.
>
>
> diff --git a/compiler-tricks/not-constant.c b/compiler-tricks/not-constant.c
> index 1da3ffc2f5..9fb4f275b1 100644
> --- a/compiler-tricks/not-constant.c
> +++ b/compiler-tricks/not-constant.c
> @@ -1,2 +1,2 @@
>  #include <git-compat-util.h>
> -int false_but_the_compiler_does_not_know_it_;
> +int false_but_the_compiler_does_not_know_it_ = 0;

Re: [PATCH/RFC v1 1/1] intialize false_but_the_compiler_does_not_know_it_

[Koji Nakamaru]

On Wed, May 7, 2025 at 2:16 AM Junio C Hamano <gitster@pobox.com> wrote:
> Just for reference (as the proposed log message refers to an "older
> macOS"), do we know if the toolchain on a more recent release of
> macOS work without this workaround already?  It may be nice to tell
> users what version they need to avoid the same issue in their own
> program.

I tested further with Xcode 14.3.1 (the last version of 14.x) and 15,
where the former still had the issue and the latter worked without the
workaround. Xcode 15 introduces a new linker which seems to fix the bug.

cf. https://developer.apple.com/documentation/xcode-release-notes/xcode-15-release-notes#Build-System

Koji Nakamaru

Re: [PATCH/RFC v1 1/1] intialize false_but_the_compiler_does_not_know_it_

[Junio C Hamano]

Koji Nakamaru <koji.nakamaru@gree.net> writes:

> On Wed, May 7, 2025 at 2:16 AM Junio C Hamano <gitster@pobox.com> wrote:
>> Just for reference (as the proposed log message refers to an "older
>> macOS"), do we know if the toolchain on a more recent release of
>> macOS work without this workaround already?  It may be nice to tell
>> users what version they need to avoid the same issue in their own
>> program.
>
> I tested further with Xcode 14.3.1 (the last version of 14.x) and 15,
> where the former still had the issue and the latter worked without the
> workaround. Xcode 15 introduces a new linker which seems to fix the bug.
>
> cf. https://developer.apple.com/documentation/xcode-release-notes/xcode-15-release-notes#Build-System
>
> Koji Nakamaru

Wonderful.  Thanks for a quick report.

Here is an updated patch with the above information.

Thanks.

--- >8 ---
From: Torsten Bögershausen <tboegi@web.de>
Date:   Tue May 6 14:06:44 2025 +0200

intialize false_but_the_compiler_does_not_know_it_

Compiling/linking 82e79c63642c on an older MacOs machine (like Xcode
14.3.1, the last version of 14.x series) leads to this:

    Undefined symbols for architecture x86_64:
      "_false_but_the_compiler_does_not_know_it_", referenced from:
          _start_command in libgit.a(run-command.o)

The linker fails to pick up compiler-tricks/not-constant.o that
defines the needed false_but_the_compiler_does_not_know_it_ symbol,
which is the only thing defined in that object file, from the
libgit.a archive.

Initializing the variable explicitly to 0 works around the linker
bug; the symbol type changes from 'C' to 'S' and is picked up by the
linker.

Xcode 15 introduces a new linker, which seems to fix the bug, making
the workaround here unnecessary, and Apple requires [*] to build with
Xcode 16 or later in order to upload to their App Store Connect
since April 24, 2025, but not everybody is expected to upgrade their
toolchain immediately.

 [*] https://developer.apple.com/news/upcoming-requirements/?id=02212025a

Helped-by: Koji Nakamaru <koji.nakamaru@gree.net>
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
[jc: update version info with Koji's help]
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/compiler-tricks/not-constant.c b/compiler-tricks/not-constant.c
index 1da3ffc2f5..9fb4f275b1 100644
--- a/compiler-tricks/not-constant.c
+++ b/compiler-tricks/not-constant.c
@@ -1,2 +1,2 @@
 #include <git-compat-util.h>
-int false_but_the_compiler_does_not_know_it_;
+int false_but_the_compiler_does_not_know_it_ = 0;