Re: [PATCH/RFC 0/3] pedantic errors in next

Carlo Marcelo Arenas Belón  [off-list ref] writes:

Building next with pedantic enabled shows the following 2 issues that
were originally in ew/many-alternate-optim, apologies for not catching
them earlier.

This of course affects 'master'.

The first two look trivially correct and I am tempted to take them
in -rc2; the last one, from my cursory look, I didn't see anything
wrong in it, but is not all that urgent, either.

the second one could be skipped, and has indeed another similar case
already in seen which will be send separately.

Thanks.

Junio C Hamano [off-list ref] wrote:

Carlo Marcelo Arenas Belón  [off-list ref] writes:

quoted

Building next with pedantic enabled shows the following 2 issues that
were originally in ew/many-alternate-optim, apologies for not catching
them earlier.

This of course affects 'master'.

The first two look trivially correct and I am tempted to take them
in -rc2; the last one, from my cursory look, I didn't see anything
wrong in it, but is not all that urgent, either.

Agreed on all counts.

I've been starting to think oidtree/cbtree would be better
done as BSD-style macro-defined functions (similar to how
khash.h is, or sys/{queue,tree}.h on *BSD systems).

I prefer Linux(kernel)-style container_of generics since I find
them easier-to-follow and have extra type-checking, but with a
flex-array it's not pedantically correct.

So I guess using CPP like khash does might be a better way to
go, here.

Thoughts?


Side note: I've also been considering Perl as a more powerful
CPP replacement so I could use the same code for a persistent
on-disk store (it would be easier to swap in pread/mmap use).
An on-disk format could make it good for refs and pre-packed
object storage (perhaps replacing loose objects).

Thanks,

in the discussion above René[1] proposed a fix for UBsan issues that were
reported and that it is still missing.

my version of it didn't require the extra 4 bytes or showed issues with
notes so is probably incomplete and should be replaced from the original
if possible, but follows below:

Carlo

[1] https://lore.kernel.org/git/bab9f889-ee2e-d3c3-0319-e297b59261a0@web.de/

+CC René for advise 

--- >8 ---

Date: Sun, 8 Aug 2021 20:45:56 -0700
Subject: [PATCH] build: fixes for SANITIZE=undefined (WIP)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

mostly from instructions/code provided by René in :

  https://lore.kernel.org/git/20210807224957.GA5068@dcvr/

tested with Xcode in macOS 11.5.1 (x86_64)
---
 hash.h        | 2 +-
 object-file.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hash.h b/hash.h
index 27a180248f..3127ba1ef8 100644
--- a/hash.h
+++ b/hash.h

@@ -115,7 +115,7 @@ static inline void git_SHA256_Clone(git_SHA256_CTX *dst, const git_SHA256_CTX *s
 
 struct object_id {
 	unsigned char hash[GIT_MAX_RAWSZ];
-	int algo;
+	uint8_t algo;
 };
 
 /* A suitably aligned type for stack allocations of hash contexts. */

diff --git a/object-file.c b/object-file.c
index 374f3c26bf..2fa282a9b4 100644
--- a/object-file.c
+++ b/object-file.c

@@ -2406,7 +2406,7 @@ struct oidtree *odb_loose_cache(struct object_directory *odb,
 	struct strbuf buf = STRBUF_INIT;
 	size_t word_bits = bitsizeof(odb->loose_objects_subdir_seen[0]);
 	size_t word_index = subdir_nr / word_bits;
-	size_t mask = 1 << (subdir_nr % word_bits);
+	size_t mask = 1U << (subdir_nr % word_bits);
 	uint32_t *bitmap;
 
 	if (subdir_nr < 0 ||

-- 
2.33.0.rc1.379.g2890ef5eb6

Am 10.08.21 um 08:16 schrieb Carlo Marcelo Arenas Belón:

Thanks,

in the discussion above René[1] proposed a fix for UBsan issues that were
reported and that it is still missing.

my version of it didn't require the extra 4 bytes or showed issues with
notes so is probably incomplete and should be replaced from the original
if possible, but follows below:

With your three patches plus the one below t3301-notes.sh and several more
fail on an Apple M1.  Adding an unused int member to struct leaf_node fixes
that.  I didn't dig deeper into the notes code to understand the actual
issue, though.

The first hunk is about alignment (and missing the notes fix, as mentioned).
The second hunk is about shifting a signed 32-bit value 31 places to the
left, which is undefined (because technically there are only 31 value bits).
Those are different issues and they should be addressed by separate patches,
I think.  That's why I submitted a patch for the the second one in
http://public-inbox.org/git/bab9f889-ee2e-d3c3-0319-e297b59261a0@web.de/.

René

On Tue, Aug 10, 2021 at 12:30 PM René Scharfe [off-list ref] wrote:

Those are different issues and they should be addressed by separate patches,
I think.  That's why I submitted a patch for the second one in
http://public-inbox.org/git/bab9f889-ee2e-d3c3-0319-e297b59261a0@web.de/.

agree, and that is why I mentioned not to merge mine but use your
whole series instead when it is published (mine was just a stopgap to
see if I could get SANITIZE=undefined to behave meanwhile, but that I
thought would be worth making public so anyone else affected might
have something to start with)

would at least the two included in the chunks above be safe enough for
RC2 as I hope?, is the one with the additional int too hacky to be
considered for release?; FWIW hadn't been able to reproduce that issue
you reported in t3301 even with an Apple M1 with macOS 11.5.1 (I use
NO_GETTEXT=1 though, not sure that might be why)

Anything I can help with?

Carlo

On Tue, Aug 10, 2021 at 4:49 PM Carlo Arenas [off-list ref] wrote:

FWIW hadn't been able to reproduce that issue
you reported in t3301 even with an Apple M1 with macOS 11.5.1 (I use
NO_GETTEXT=1 though, not sure that might be why)

but it seems to be reproducible in the 32bit linux job from CI[1] :

  git: notes.c:206: note_tree_remove: Assertion `GET_PTR_TYPE(entry)
== 0' failed.

Carlo

[1] https://github.com/carenas/git/runs/3295672049?check_suite_focus=true

Hi Carlo,

Am 11.08.21 um 01:49 schrieb Carlo Arenas:

On Tue, Aug 10, 2021 at 12:30 PM René Scharfe [off-list ref] wrote:

quoted

Those are different issues and they should be addressed by separate patches,
I think.  That's why I submitted a patch for the second one in
http://public-inbox.org/git/bab9f889-ee2e-d3c3-0319-e297b59261a0@web.de/.

agree, and that is why I mentioned not to merge mine but use your
whole series instead when it is published (mine was just a stopgap to
see if I could get SANITIZE=undefined to behave meanwhile, but that I
thought would be worth making public so anyone else affected might
have something to start with)

would at least the two included in the chunks above be safe enough for
RC2 as I hope?, is the one with the additional int too hacky to be
considered for release?;

I think your -pedantic fixes 1 and 2 should go into the next possible
release candidate because they fix regressions.

Same for my signed-left-shift fix in
http://public-inbox.org/git/bab9f889-ee2e-d3c3-0319-e297b59261a0@web.de/
(or some improved version if it's lacking in some way) and the yet to be
published fix for the alignment issue.  I assume Andrzej as the reporter
or Eric as the original author would like to have a shot at the latter.

FWIW hadn't been able to reproduce that issue
you reported in t3301 even with an Apple M1 with macOS 11.5.1 (I use
NO_GETTEXT=1 though, not sure that might be why)

Strange.  I use Apple clang version 12.0.5 (clang-1205.0.22.11) on the
same OS.

At least the reproduction on Linux that you mentioned in your reply
means I can calm down a bit because it's not just a problem on my
system..

Thank you,
René