[RFC/PATCH,v1,0/4] compat/obstack: update from upstream
mbox series

Message ID 20190614100059.13540-1-szeder.dev@gmail.com
Headers show
Series
  • compat/obstack: update from upstream
Related show

Message

SZEDER Gábor June 14, 2019, 10 a.m. UTC
Update 'compat/obstack.{c,h}' from upstream, because they already use
'size_t' instead of 'long' in places that might eventually end up as
an argument to malloc(), which might solve build errors with GCC 8 on
Windows.

The first patch just imports from upstream and doesn't modify anything
at all, and, consequently, it can't be compiled because of a screenful
or two of errors.  This is bad for future bisects, of course.

OTOH, adding all the necessary build fixes right away makes review
harder...

I'm not sure how to deal with this situation, so here is a series with
the fixes in separate patches for review, for now.  If there's an
agreement that this is the direction to take, then I'll squash in the
fixes in the first patch and touch up the resulting commit message.


Ramsay, could you please run sparse on top of these patch series to
make sure that I caught and converted all "0 instead of NULL" usages
in the last patch?  Thanks.


And here is an all-green build of these patches on Travis CI:

  https://travis-ci.org/szeder/git/builds/545645247

(and one bonus patch on top to deal with some Homebrew nonsense)

SZEDER Gábor (4):
  compat/obstack: update from upstream
  SQUASH??? compat/obstack: fix portability issues
  SQUASH??? compat/obstack: fix build errors with Clang
  compat/obstack: fix some sparse warnings

 compat/obstack.c | 356 ++++++++------------
 compat/obstack.h | 832 ++++++++++++++++++++++++-----------------------
 2 files changed, 572 insertions(+), 616 deletions(-)

Comments

Jeff King June 14, 2019, 5:57 p.m. UTC | #1
On Fri, Jun 14, 2019 at 12:00:55PM +0200, SZEDER Gábor wrote:

> Update 'compat/obstack.{c,h}' from upstream, because they already use
> 'size_t' instead of 'long' in places that might eventually end up as
> an argument to malloc(), which might solve build errors with GCC 8 on
> Windows.
> 
> The first patch just imports from upstream and doesn't modify anything
> at all, and, consequently, it can't be compiled because of a screenful
> or two of errors.  This is bad for future bisects, of course.
> 
> OTOH, adding all the necessary build fixes right away makes review
> harder...

One thing about your approach that makes it hard to review is that the
first commit obliterates all of our local changes, and then you have to
re-apply them individually. Looking at "git log" there aren't very many
in this case, so it's pretty easy to be sure you got them all (in some
cases this can be particularly nasty if the changes were themselves part
of conflict resolution, and so you have to pick them out of a merge).

I think a flow that better matches "what really happened" is to do more
of a vendor-branch approach: have a line of history that represents the
upstream changes (each one obliterating the last), and then periodically
merge that into our fork.

That can even retain bisectability as long as the commits along the
vendor branch don't actually try to build the code. Unfortunately our
initial import does try to build, so I think it already wrecks
bisectability, but we can undo that now. So e.g.,:

  # start at e831171d67 (Add obstack.[ch] from EGLIBC 2.10, 2011-08-21)
  git checkout -b upstream-obstack e831171d67

  # undo build changes to restore bisection; ideally this would have
  # been done back then, but it's too late now
  sed -i /obstack/d Makefile
  git commit -am 'strip out obstack building'

  # but of course in our merged version we want that back, so let's
  # do a noop merge to represent that.
  git checkout master ;# or whatever feature branch you're working on
  git merge -s ours upstream-obstack

  # and now with a sane vendor branch established, we can proceed to do
  # a real update there
  git checkout upstream-obstack
  cp /path/to/obstack.[ch] compat/
  git commit -am 'update obstack'

  # and now we are free to merge that in, getting a real 3-way merge
  # between our changes and what happened upstream.
  git checkout master
  git merge upstream-obstack

Now, if you try this you may find that the conflicts are pretty horrid.
And the result may end up way less readable than your cherry-picks (and
harder to resolve in the first place). I claim only that:

  1. This represents in the history graph more clearly the actual
     sequence of events.

  2. Its saves you from digging up the set of changes that have been
     applied since our last upstream import.

So in this case the way you did it may well be the best way. But I offer
it as an alternative. :)

-Peff
Junio C Hamano June 14, 2019, 6:19 p.m. UTC | #2
SZEDER Gábor <szeder.dev@gmail.com> writes:

> And here is an all-green build of these patches on Travis CI:
>
>   https://travis-ci.org/szeder/git/builds/545645247
>
> (and one bonus patch on top to deal with some Homebrew nonsense)

Is this the one that making all of the jobs pass in the above
output, including the mac gcc one.  It would be wonderful to have it
separately and fast-tracked ;-)

-- >8 --
From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Wed, 3 Apr 2019 02:49:47 +0200
Subject: [PATCH] ci: make Homebrew's operations faster

---
 ci/install-dependencies.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 7f6acdd803..f804b40ddd 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -34,7 +34,7 @@ linux-clang|linux-gcc)
 	popd
 	;;
 osx-clang|osx-gcc)
-	brew update >/dev/null
+	export HOMEBREW_NO_INSTALL_CLEANUP=1 HOMEBREW_NO_AUTO_UPDATE=1
 	# Uncomment this if you want to run perf tests:
 	# brew install gnu-time
 	test -z "$BREW_INSTALL_PACKAGES" ||
Ramsay Jones June 14, 2019, 8:30 p.m. UTC | #3
On 14/06/2019 11:00, SZEDER Gábor wrote:
> Update 'compat/obstack.{c,h}' from upstream, because they already use
> 'size_t' instead of 'long' in places that might eventually end up as
> an argument to malloc(), which might solve build errors with GCC 8 on
> Windows.
> 
> The first patch just imports from upstream and doesn't modify anything
> at all, and, consequently, it can't be compiled because of a screenful
> or two of errors.  This is bad for future bisects, of course.
> 
> OTOH, adding all the necessary build fixes right away makes review
> harder...
> 
> I'm not sure how to deal with this situation, so here is a series with
> the fixes in separate patches for review, for now.  If there's an
> agreement that this is the direction to take, then I'll squash in the
> fixes in the first patch and touch up the resulting commit message.
> 
> 
> Ramsay, could you please run sparse on top of these patch series to
> make sure that I caught and converted all "0 instead of NULL" usages
> in the last patch?  Thanks.

I applied your patches to current master (@0aae918dd9) and, since
you dropped the final hunk of commit 3254310863 ("obstack.c: Fix
some sparse warnings", 2011-09-11), sparse complains, thus:

  $ diff sp-out sp-out1
  27a28,30
  > compat/obstack.c:331:5: warning: incorrect type in initializer (different modifiers)
  > compat/obstack.c:331:5:    expected void ( *[addressable] [toplevel] obstack_alloc_failed_handler )( ... )
  > compat/obstack.c:331:5:    got void ( [noreturn] * )( ... )
  $ 

So, yes you did catch all "using plain integer as NULL pointer"
warnings! :-D

Thanks.

ATB,
Ramsay Jones
Ramsay Jones June 14, 2019, 9:24 p.m. UTC | #4
On 14/06/2019 21:30, Ramsay Jones wrote:
> 
> 
> On 14/06/2019 11:00, SZEDER Gábor wrote:
>> Update 'compat/obstack.{c,h}' from upstream, because they already use
>> 'size_t' instead of 'long' in places that might eventually end up as
>> an argument to malloc(), which might solve build errors with GCC 8 on
>> Windows.
>>
>> The first patch just imports from upstream and doesn't modify anything
>> at all, and, consequently, it can't be compiled because of a screenful
>> or two of errors.  This is bad for future bisects, of course.
>>
>> OTOH, adding all the necessary build fixes right away makes review
>> harder...
>>
>> I'm not sure how to deal with this situation, so here is a series with
>> the fixes in separate patches for review, for now.  If there's an
>> agreement that this is the direction to take, then I'll squash in the
>> fixes in the first patch and touch up the resulting commit message.
>>
>>
>> Ramsay, could you please run sparse on top of these patch series to
>> make sure that I caught and converted all "0 instead of NULL" usages
>> in the last patch?  Thanks.
> 
> I applied your patches to current master (@0aae918dd9) and, since
> you dropped the final hunk of commit 3254310863 ("obstack.c: Fix
> some sparse warnings", 2011-09-11), sparse complains, thus:
> 
>   $ diff sp-out sp-out1
>   27a28,30
>   > compat/obstack.c:331:5: warning: incorrect type in initializer (different modifiers)
>   > compat/obstack.c:331:5:    expected void ( *[addressable] [toplevel] obstack_alloc_failed_handler )( ... )
>   > compat/obstack.c:331:5:    got void ( [noreturn] * )( ... )
>   $ 
> 
> So, yes you did catch all "using plain integer as NULL pointer"
> warnings! :-D

Sorry for being a bit slow here, but I just realized that
I should not have seen that on Linux (and should have tested
on cygwin), because the obstack code gets elided on Linux ...

Oh, wait:

  $ diff sc sc1
  3a4,7
  > compat/obstack.o	- _obstack_allocated_p
  > compat/obstack.o	- obstack_alloc_failed_handler
  > compat/obstack.o	- _obstack_begin_1
  > compat/obstack.o	- _obstack_memory_used
  $ 

Hmm, so on master, this code is totally elided on Linux, but
that is no longer the case with your patches applied. I guess
you need to look at the definition of the {_OBSTACK_}ELIDE_CODE
preprocessor variable(s).

HTH.

ATB,
Ramsay Jones
SZEDER Gábor June 17, 2019, 6:36 p.m. UTC | #5
On Fri, Jun 14, 2019 at 09:30:20PM +0100, Ramsay Jones wrote:
> 
> 
> On 14/06/2019 11:00, SZEDER Gábor wrote:
> > Update 'compat/obstack.{c,h}' from upstream, because they already use
> > 'size_t' instead of 'long' in places that might eventually end up as
> > an argument to malloc(), which might solve build errors with GCC 8 on
> > Windows.
> > 
> > The first patch just imports from upstream and doesn't modify anything
> > at all, and, consequently, it can't be compiled because of a screenful
> > or two of errors.  This is bad for future bisects, of course.
> > 
> > OTOH, adding all the necessary build fixes right away makes review
> > harder...
> > 
> > I'm not sure how to deal with this situation, so here is a series with
> > the fixes in separate patches for review, for now.  If there's an
> > agreement that this is the direction to take, then I'll squash in the
> > fixes in the first patch and touch up the resulting commit message.
> > 
> > 
> > Ramsay, could you please run sparse on top of these patch series to
> > make sure that I caught and converted all "0 instead of NULL" usages
> > in the last patch?  Thanks.
> 
> I applied your patches to current master (@0aae918dd9) and, since
> you dropped the final hunk of commit 3254310863 ("obstack.c: Fix
> some sparse warnings", 2011-09-11), sparse complains, thus:

Oh, indeed.  3254310863 removed that "__attribute__ ((noreturn))" from
the function's definition, but nowadays upstream writes that as
"static _Noreturn void print_and_abort (void)", and I didn't realize
that this _Noreturn is the same thing.

>   $ diff sp-out sp-out1
>   27a28,30
>   > compat/obstack.c:331:5: warning: incorrect type in initializer (different modifiers)
>   > compat/obstack.c:331:5:    expected void ( *[addressable] [toplevel] obstack_alloc_failed_handler )( ... )
>   > compat/obstack.c:331:5:    got void ( [noreturn] * )( ... )
>   $ 
> 
> So, yes you did catch all "using plain integer as NULL pointer"
> warnings! :-D

Heh :)

Anyway, I won't do anything for the time being, in the hope that we
can get on board with removing kwset/obstack...