Re: SunCC doesn't compile v2.32.0-rc* anymore (was "Re: [PATCH 2/3] t/helper/test-bitmap.c: initial commit")

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, May 26 2021, Taylor Blau wrote:

> On Wed, May 26, 2021 at 08:30:49PM +0200, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Wed, Mar 31 2021, Taylor Blau wrote:
>>
>> > Add a new 'bitmap' test-tool which can be used to list the commits that
>> > have received bitmaps.
>> > [...]
>> > @@ -0,0 +1,24 @@
>> > +#include "test-tool.h"
>> > +#include "cache.h"
>> > +#include "pack-bitmap.h"
>>
>> Since this commit SunCC (on Solaris) refuses to compile git, a new bug
>> in v2.32.0-rc*.
>>
>> It's because it can't find the oe_get_size_slow symbol, you include
>> pack-bitmap.h, which in turn includes pack-objects.h. That file
>> references oe_get_size_slow, but it's only defined in
>> builtin/pack-objects.c.
>
> I'm not sure I understand. pack-objects.h has a declaration of that
> method, but the implementation is in builtin/pack-objects.c. That should
> be fine, but I don't know about how SunCC works.
>
> What needs to be changed here?

I think that oe_get_size_slow needs to be in libgit as long as
pack-objects.h defines other (inline) functions that reference it, or
maybe most of that stuff can just be moved to builtin/pack-objects.h?

This obviously not OK patch makes things "ok" again under SunCC:
    
    diff --git a/Makefile b/Makefile
    index c3565fc0f8f..c2b05aeddac 100644
    --- a/Makefile
    +++ b/Makefile
    @@ -1186,7 +1186,7 @@ THIRD_PARTY_SOURCES += compat/regex/%
     THIRD_PARTY_SOURCES += sha1collisiondetection/%
     THIRD_PARTY_SOURCES += sha1dc/%
    
    -GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB)
    +GITLIBS = builtin/pack-objects.o common-main.o $(LIB_FILE) $(XDIFF_LIB)
     EXTLIBS =
    
     GIT_USER_AGENT = git/$(GIT_VERSION)

I.e. the problem is that in the final program we're linking there's
references to this function we can't find:
    
    Undefined                       first referenced
     symbol                             in file
    oe_get_size_slow                    t/helper/test-bitmap.o
    ld: fatal: symbol referencing errors. No output written to t/helper/test-tool

I think e.g. the GNU toolchain and others are probably OK with it
because they do some analysis to figure out that none of those inline
functions that need oe_get_size_slow are themselves needed. SunCC (or
Solaris's) linker seems to be more eager.

It seems to me that the best way to solve this is something like the
below code-move-only patch of just moving this stuff only used by
builtin/pack-objects.c to that file. This also fixes the build on
Solaris.

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 6ded130e45b..70072b07b6b 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -37,6 +37,100 @@
 #include "shallow.h"
 #include "promisor-remote.h"
 
+/*
+ * Objects we are going to pack are collected in the `to_pack` structure.
+ * It contains an array (dynamically expanded) of the object data, and a map
+ * that can resolve SHA1s to their position in the array.
+ */
+static struct packing_data to_pack;
+
+/*
+ * Return the size of the object without doing any delta
+ * reconstruction (so non-deltas are true object sizes, but deltas
+ * return the size of the delta data).
+ */
+unsigned long oe_get_size_slow(struct packing_data *pack,
+			       const struct object_entry *e);
+unsigned long oe_get_size_slow(struct packing_data *pack,
+			       const struct object_entry *e)
+{
+	struct packed_git *p;
+	struct pack_window *w_curs;
+	unsigned char *buf;
+	enum object_type type;
+	unsigned long used, avail, size;
+
+	if (e->type_ != OBJ_OFS_DELTA && e->type_ != OBJ_REF_DELTA) {
+		packing_data_lock(&to_pack);
+		if (oid_object_info(the_repository, &e->idx.oid, &size) < 0)
+			die(_("unable to get size of %s"),
+			    oid_to_hex(&e->idx.oid));
+		packing_data_unlock(&to_pack);
+		return size;
+	}
+
+	p = oe_in_pack(pack, e);
+	if (!p)
+		BUG("when e->type is a delta, it must belong to a pack");
+
+	packing_data_lock(&to_pack);
+	w_curs = NULL;
+	buf = use_pack(p, &w_curs, e->in_pack_offset, &avail);
+	used = unpack_object_header_buffer(buf, avail, &type, &size);
+	if (used == 0)
+		die(_("unable to parse object header of %s"),
+		    oid_to_hex(&e->idx.oid));
+
+	unuse_pack(&w_curs);
+	packing_data_unlock(&to_pack);
+	return size;
+}
+
+static inline unsigned long oe_size(struct packing_data *pack,
+				    const struct object_entry *e)
+{
+	if (e->size_valid)
+		return e->size_;
+
+	return oe_get_size_slow(pack, e);
+}
+
+static inline int oe_size_less_than(struct packing_data *pack,
+				    const struct object_entry *lhs,
+				    unsigned long rhs)
+{
+	if (lhs->size_valid)
+		return lhs->size_ < rhs;
+	if (rhs < pack->oe_size_limit) /* rhs < 2^x <= lhs ? */
+		return 0;
+	return oe_get_size_slow(pack, lhs) < rhs;
+}
+
+static inline int oe_size_greater_than(struct packing_data *pack,
+				       const struct object_entry *lhs,
+				       unsigned long rhs)
+{
+	if (lhs->size_valid)
+		return lhs->size_ > rhs;
+	if (rhs < pack->oe_size_limit) /* rhs < 2^x <= lhs ? */
+		return 1;
+	return oe_get_size_slow(pack, lhs) > rhs;
+}
+
+static inline void oe_set_size(struct packing_data *pack,
+			       struct object_entry *e,
+			       unsigned long size)
+{
+	if (size < pack->oe_size_limit) {
+		e->size_ = size;
+		e->size_valid = 1;
+	} else {
+		e->size_valid = 0;
+		if (oe_get_size_slow(pack, e) != size)
+			BUG("'size' is supposed to be the object size!");
+	}
+}
+
 #define IN_PACK(obj) oe_in_pack(&to_pack, obj)
 #define SIZE(obj) oe_size(&to_pack, obj)
 #define SET_SIZE(obj,size) oe_set_size(&to_pack, obj, size)
@@ -56,13 +150,6 @@ static const char *pack_usage[] = {
 	NULL
 };
 
-/*
- * Objects we are going to pack are collected in the `to_pack` structure.
- * It contains an array (dynamically expanded) of the object data, and a map
- * that can resolve SHA1s to their position in the array.
- */
-static struct packing_data to_pack;
-
 static struct pack_idx_entry **written_list;
 static uint32_t nr_result, nr_written, nr_seen;
 static struct bitmap_index *bitmap_git;
@@ -2231,46 +2318,6 @@ static pthread_mutex_t progress_mutex;
  * progress_mutex for protection.
  */
 
-/*
- * Return the size of the object without doing any delta
- * reconstruction (so non-deltas are true object sizes, but deltas
- * return the size of the delta data).
- */
-unsigned long oe_get_size_slow(struct packing_data *pack,
-			       const struct object_entry *e)
-{
-	struct packed_git *p;
-	struct pack_window *w_curs;
-	unsigned char *buf;
-	enum object_type type;
-	unsigned long used, avail, size;
-
-	if (e->type_ != OBJ_OFS_DELTA && e->type_ != OBJ_REF_DELTA) {
-		packing_data_lock(&to_pack);
-		if (oid_object_info(the_repository, &e->idx.oid, &size) < 0)
-			die(_("unable to get size of %s"),
-			    oid_to_hex(&e->idx.oid));
-		packing_data_unlock(&to_pack);
-		return size;
-	}
-
-	p = oe_in_pack(pack, e);
-	if (!p)
-		BUG("when e->type is a delta, it must belong to a pack");
-
-	packing_data_lock(&to_pack);
-	w_curs = NULL;
-	buf = use_pack(p, &w_curs, e->in_pack_offset, &avail);
-	used = unpack_object_header_buffer(buf, avail, &type, &size);
-	if (used == 0)
-		die(_("unable to parse object header of %s"),
-		    oid_to_hex(&e->idx.oid));
-
-	unuse_pack(&w_curs);
-	packing_data_unlock(&to_pack);
-	return size;
-}
-
 static int try_delta(struct unpacked *trg, struct unpacked *src,
 		     unsigned max_depth, unsigned long *mem_usage)
 {
diff --git a/pack-objects.h b/pack-objects.h
index 9d88e3e518f..0c4d5d475f6 100644
--- a/pack-objects.h
+++ b/pack-objects.h
@@ -332,52 +332,6 @@ static inline void oe_set_delta_sibling(struct packing_data *pack,
 		e->delta_sibling_idx = 0;
 }
 
-unsigned long oe_get_size_slow(struct packing_data *pack,
-			       const struct object_entry *e);
-static inline unsigned long oe_size(struct packing_data *pack,
-				    const struct object_entry *e)
-{
-	if (e->size_valid)
-		return e->size_;
-
-	return oe_get_size_slow(pack, e);
-}
-
-static inline int oe_size_less_than(struct packing_data *pack,
-				    const struct object_entry *lhs,
-				    unsigned long rhs)
-{
-	if (lhs->size_valid)
-		return lhs->size_ < rhs;
-	if (rhs < pack->oe_size_limit) /* rhs < 2^x <= lhs ? */
-		return 0;
-	return oe_get_size_slow(pack, lhs) < rhs;
-}
-
-static inline int oe_size_greater_than(struct packing_data *pack,
-				       const struct object_entry *lhs,
-				       unsigned long rhs)
-{
-	if (lhs->size_valid)
-		return lhs->size_ > rhs;
-	if (rhs < pack->oe_size_limit) /* rhs < 2^x <= lhs ? */
-		return 1;
-	return oe_get_size_slow(pack, lhs) > rhs;
-}
-
-static inline void oe_set_size(struct packing_data *pack,
-			       struct object_entry *e,
-			       unsigned long size)
-{
-	if (size < pack->oe_size_limit) {
-		e->size_ = size;
-		e->size_valid = 1;
-	} else {
-		e->size_valid = 0;
-		if (oe_get_size_slow(pack, e) != size)
-			BUG("'size' is supposed to be the object size!");
-	}
-}
 
 static inline unsigned long oe_delta_size(struct packing_data *pack,
 					  const struct object_entry *e)