On 07/07/2022 12:17, Ævar Arnfjörð Bjarmason wrote:
On Wed, Jul 06 2022, Phillip Wood wrote:
Hi Ævar
On 30/06/2022 14:25, Ævar Arnfjörð Bjarmason wrote:
On Thu, Jun 30 2022, Phillip Wood wrote:
On 30/06/2022 13:03, Phillip Wood wrote:
On 30/06/2022 11:54, Ævar Arnfjörð Bjarmason wrote:
On Wed, Jun 29 2022, Phillip Wood via GitGitGadget wrote:
From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
Add a helper to grow an array. This is analogous to ALLOC_GROW() in
the rest of the codebase but returns −1 on allocation failure to
accommodate other users of libxdiff such as libgit2.
Urm, does it? I just skimmed this, so maybe I missed something, but I
don't see where you changed the definition of xdl_malloc(),
xdl_realloc() etc.
Oh I think I might have misunderstood your question. For git.git it
will still die() but for other users that arrange for xdl_realloc() to
return NULL on failure it will return -1. The same applies to the
comments in the previous two patches about XDL_[CM]ALLOC_ARRAY()
returning NULL on allocation failure.
Yes, I meant that the "but returns −1 on allocation failure to
accommodate other users of libxdiff such as libgit2" is really more of
a:
...but *allows for* dropping in another xmalloc(), xrealloc()
etc. implementation that doesn't die on failure.
So I think the rest of my upthread question still stands, i.e.:
"So if that's the plan why would we need an XDL_ALLOC_ARRAY(),
can't you just check that it [I mean ALLOC_ARRAY()] returns
non-NULL?"
I.e. if the plan is to replace the underlying x*() functions with
non-fatal variants can't you use ALLOC_ARRAY() instead? I haven't tried
that, but I don't see a reason we couldn't do that in principle...
As the cover letter says, the aim of this series is not to replace
xmalloc() etc with non-fatal variants, it is just to make the xdiff
code more readable.
I don't think it's more readable to carry code in-tree that's
unreachable except when combined with code out-of-tree. I.e. this series
leaves us with the equivalent of:
ptr = xmalloc(...);
if (!ptr)
/* unreachable in git.git ... */
I don't think it's more readable to have code that rather trivial
analysis will show goes against the "__attribute__((noreturn))" we're
placing on our die() function.
We're already in this situation. The code in xdiff is written to handle
allocation failures and we use an allocation function that dies instead.
This patch series does nothing to alter that situation.
Which is what I'm pointing out with "running with scissors". I.e. I'm
fully on-board with the end goal, but that can be accomplished in a way
that doesn't confuse humans & analyzers alike.
(One can already use a non-fatal allocation
function for xdl_malloc()) [...]
That just seems inviting a segfault or undefined/untested behavior
(whether in the sense of "undefined by C" or "untested by git.git's
codebase logic"). Everything around xmalloc() now assumes "never returns
NULL", and you want to:
* Make it return NULL when combined with out-of-tree-code
No I do not want to alter the behavior of xmalloc() at all, that is why
this series does not alter the behavior of xmalloc()
* Maintain the code in git.git, where it never returns NULL, but in a
way where we won't have bugs when combined with a new macro that
behaves differently, in a way we never even test ourselves.
That describes the current situation with xdiff, this series does not
alter that.
Isn't that correct, or am I missing something?
You should note that libgit2 uses malloc() as it's default allocator,
seeming without issue.
I don't think that using ALLOC_ARRAY() in
xdiff is helpful for other users as they would have to define their
own array allocation macros, rather than just providing their own
allocation functions. I would like to reduce the friction others have
upstreaming xdiff patches to us, not increase it.
Yes, I'm totally on-board with reducing the friction in others using
xdiff, and would like to see more of that sort of out-of-tree use in
general (although for things outside of xdiff GPL v.s. LGPL concerns
come into play).
I'd even like for us to explicitly make that much easier. I.e. if you
want to use xdiff now you search for it, and find the at this point
unmaintained upstream, and if you find that git has a "newer" version
you'll have some trouble extracting it already.
After this series you'll need to start writing & maintaining your own
non-trivial alloc wrapper logic if you're doing that. If you get it
subtly wrong you'll have a buggy xdiff, and most likely users will just
copy/paste the git.git version from our git-compat-util.h & cache.h,
which is rather silly.
This series does not alter what wrappers you need to write whereas your
suggestion of using ALLOC_ARRAY() would force more work on potential
xdiff users (though below I think you're suggesting we provide them in a
separate header so they can be reused more easily).
Which is why I'm saying we could/should do this in a much easier way,
i.e.:
* Factor out the now only-fatal-on-NULL ALLOC_GROW() such that we can
have a non-fatal version (ALLOC_GROW) and a non-fatal one.
I don't know what we'd call it. we usually have X*() meaning "fatal",
but these are fatal by default, maybe G* for gently? So
GALLOC_GROW(). Urgh, anyway, continuing with that ugly name...
Further proof that naming is hard...
* Have xdiff/ use that GALLOC_GROW() & malloc(), not ALLOC_GROW() &
xmalloc(), as we really want to have the appropriate code flow
analysis etc. spot for us that we should handle NULL returns,
otherwise combining this code with libgit2 will be buggy/broken.
This makes it much easier for libgit2 to use this, as it won't need to
do anything special at all. Since our GALLOC_GROW() will eventualy use
malloc() instead of xmalloc() you don't need to define anything that
re-implements the GALLOC_GROW() or whatever other non-fatal versions of
our only-fatal helpers we have.
This assumes that we'd move these macros out of git-compat-util.h and a
new git-exernal-compat.h, or that instead of *just* copying the xdiff/
directory your import script would need to run some small bit of cc -E
and or perl/sed to one-off extract the smell bits of
git-exernal-compat.h or cache.h that we need.
I think there is an argument that we should change our xdiff wrapper to
use malloc() rather than xmalloc() so we're able to test the error
handling. That then begs the question as to how we actually get the
allocation functions to fail when they're being tested. I also think
that is an orthogonal change that could happen with or without this
patch series.
Best Wishes
Phillip
