[RFC,1/6] common-main: split common_exit() into a new file

Commit Message

Josh Steadmon Aug. 7, 2024, 6:21 p.m. UTC

Currently, object files in libgit.a reference common_exit(), which is
contained in common-main.o. However, common-main.o also includes main(),
which references cmd_main() in git.o, which in turn depends on all the
builtin/*.o objects.

We would like to allow external users to link libgit.a without needing
to include so many extra objects. Enable this by splitting common_exit()
and check_bug_if_BUG() into a new file common-exit.c, and add
common-exit.o to LIB_OBJS so that these are included in libgit.a.

This split has previously been proposed ([1], [2]) to support fuzz tests
and unit tests by avoiding conflicting definitions for main(). However,
both of those issues were resolved by other methods of avoiding symbol
conflicts. Now we are trying to make libgit.a more self-contained, so
hopefully we can revisit this approach.

[1] https://lore.kernel.org/git/[e-mail]/
[2] https://lore.kernel.org/git/[e-mail]/

Signed-off-by: Josh Steadmon <[e-mail]>
---
 Makefile      |  1 +
 common-exit.c | 26 ++++++++++++++++++++++++++
 common-main.c | 24 ------------------------
 3 files changed, 27 insertions(+), 24 deletions(-)
 create mode 100644 common-exit.c

Comments

Junio C Hamano Aug. 7, 2024, 9:21 p.m. UTC | #1

Josh Steadmon <[e-mail]> writes:

> diff --git a/common-exit.c b/common-exit.c
> new file mode 100644
> index 0000000000..1aaa538be3
> --- /dev/null
> +++ b/common-exit.c
> @@ -0,0 +1,26 @@
> +#include "git-compat-util.h"
> +#include "trace2.h"
> +
> +static void check_bug_if_BUG(void)
> +{
> +	if (!bug_called_must_BUG)
> +		return;
> +	BUG("on exit(): had bug() call(s) in this process without explicit BUG_if_bug()");
> +}

Nice that this can stay file-scope static.

> +/* We wrap exit() to call common_exit() in git-compat-util.h */
> +int common_exit(const char *file, int line, int code)
> +{
> +	/*
> +	 * For non-POSIX systems: Take the lowest 8 bits of the "code"
> +	 * to e.g. turn -1 into 255. On a POSIX system this is
> +	 * redundant, see exit(3) and wait(2), but as it doesn't harm
> +	 * anything there we don't need to guard this with an "ifdef".
> +	 */
> +	code &= 0xff;
> +
> +	check_bug_if_BUG();
> +	trace2_cmd_exit_fl(file, line, code);
> +
> +	return code;
> +}

I wonder if at least the part that primes the trace2 system needs to
also be split out of the file that defines our own main(), though.

Are libgit.a users now responsible for calling things like
trace2_initialize_clock(), trace2_initialize(), etc., themselves?  I
am wondering if these calls are encapsulated into a simple helper
function, say common_startup(), then the story we need to tell the
libgit.a users may become simpler, i.e. you call common_startup, do
your things, and then you somehow cause common_exit() to be called.

Josh Steadmon Aug. 7, 2024, 10:54 p.m. UTC | #2

On 2024.08.07 14:21, Junio C Hamano wrote:
> Josh Steadmon <[e-mail]> writes:
> 
> > diff --git a/common-exit.c b/common-exit.c
> > new file mode 100644
> > index 0000000000..1aaa538be3
> > --- /dev/null
> > +++ b/common-exit.c
> > @@ -0,0 +1,26 @@
> > +#include "git-compat-util.h"
> > +#include "trace2.h"
> > +
> > +static void check_bug_if_BUG(void)
> > +{
> > +	if (!bug_called_must_BUG)
> > +		return;
> > +	BUG("on exit(): had bug() call(s) in this process without explicit BUG_if_bug()");
> > +}
> 
> Nice that this can stay file-scope static.
> 
> > +/* We wrap exit() to call common_exit() in git-compat-util.h */
> > +int common_exit(const char *file, int line, int code)
> > +{
> > +	/*
> > +	 * For non-POSIX systems: Take the lowest 8 bits of the "code"
> > +	 * to e.g. turn -1 into 255. On a POSIX system this is
> > +	 * redundant, see exit(3) and wait(2), but as it doesn't harm
> > +	 * anything there we don't need to guard this with an "ifdef".
> > +	 */
> > +	code &= 0xff;
> > +
> > +	check_bug_if_BUG();
> > +	trace2_cmd_exit_fl(file, line, code);
> > +
> > +	return code;
> > +}
> 
> I wonder if at least the part that primes the trace2 system needs to
> also be split out of the file that defines our own main(), though.
> 
> Are libgit.a users now responsible for calling things like
> trace2_initialize_clock(), trace2_initialize(), etc., themselves?  I
> am wondering if these calls are encapsulated into a simple helper
> function, say common_startup(), then the story we need to tell the
> libgit.a users may become simpler, i.e. you call common_startup, do
> your things, and then you somehow cause common_exit() to be called.

Yeah, I actually already have a fixup patch to move the initialization
into a separate object file as well. Will include in V2.

Patch

diff --git a/Makefile b/Makefile
index 3eab701b10..1cac51a4f7 100644
--- a/Makefile
+++ b/Makefile
@@ -979,6 +979,7 @@  LIB_OBJS += combine-diff.o
 LIB_OBJS += commit-graph.o
 LIB_OBJS += commit-reach.o
 LIB_OBJS += commit.o
+LIB_OBJS += common-exit.o
 LIB_OBJS += compat/nonblock.o
 LIB_OBJS += compat/obstack.o
 LIB_OBJS += compat/terminal.o
diff --git a/common-exit.c b/common-exit.c
new file mode 100644
index 0000000000..1aaa538be3
--- /dev/null
+++ b/common-exit.c
@@ -0,0 +1,26 @@ 
+#include "git-compat-util.h"
+#include "trace2.h"
+
+static void check_bug_if_BUG(void)
+{
+	if (!bug_called_must_BUG)
+		return;
+	BUG("on exit(): had bug() call(s) in this process without explicit BUG_if_bug()");
+}
+
+/* We wrap exit() to call common_exit() in git-compat-util.h */
+int common_exit(const char *file, int line, int code)
+{
+	/*
+	 * For non-POSIX systems: Take the lowest 8 bits of the "code"
+	 * to e.g. turn -1 into 255. On a POSIX system this is
+	 * redundant, see exit(3) and wait(2), but as it doesn't harm
+	 * anything there we don't need to guard this with an "ifdef".
+	 */
+	code &= 0xff;
+
+	check_bug_if_BUG();
+	trace2_cmd_exit_fl(file, line, code);
+
+	return code;
+}
diff --git a/common-main.c b/common-main.c
index 8e68ac9e42..af4dea049e 100644
--- a/common-main.c
+++ b/common-main.c
@@ -66,27 +66,3 @@  int main(int argc, const char **argv)
 	/* Not exit(3), but a wrapper calling our common_exit() */
 	exit(result);
 }
-
-static void check_bug_if_BUG(void)
-{
-	if (!bug_called_must_BUG)
-		return;
-	BUG("on exit(): had bug() call(s) in this process without explicit BUG_if_bug()");
-}
-
-/* We wrap exit() to call common_exit() in git-compat-util.h */
-int common_exit(const char *file, int line, int code)
-{
-	/*
-	 * For non-POSIX systems: Take the lowest 8 bits of the "code"
-	 * to e.g. turn -1 into 255. On a POSIX system this is
-	 * redundant, see exit(3) and wait(2), but as it doesn't harm
-	 * anything there we don't need to guard this with an "ifdef".
-	 */
-	code &= 0xff;
-
-	check_bug_if_BUG();
-	trace2_cmd_exit_fl(file, line, code);
-
-	return code;
-}