Freeing struct lock_file?

Freeing struct lock_file?

[David Turner]

Why is it impossible to free struct lock_files?  I understand that they
become part of a linked list, and that there's an atexit handler that
goes over that list.  But couldn't we just remove them from the linked
list and then free them?  Even if we couldn't free all lock_files, maybe
we could free some?  Anyway, just curious why that decision was made.

Re: Freeing struct lock_file?

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> Why is it impossible to free struct lock_files?  I understand that they
> become part of a linked list, and that there's an atexit handler that
> goes over that list.  But couldn't we just remove them from the linked
> list and then free them? 

I suspect that the code is worried about getting a signal, while it
is manipulating the linked list, and then cause the atexit handler
to walk a list that is in a broken state.

Re: Freeing struct lock_file?

[David Turner]

On Fri, 2015-04-03 at 15:01 -0700, Junio C Hamano wrote:
> David Turner <dturner@twopensource.com> writes:
> 
> > Why is it impossible to free struct lock_files?  I understand that they
> > become part of a linked list, and that there's an atexit handler that
> > goes over that list.  But couldn't we just remove them from the linked
> > list and then free them? 
> 
> I suspect that the code is worried about getting a signal, while it
> is manipulating the linked list, and then cause the atexit handler
> to walk a list that is in a broken state.

This is technically possible, but practically unlikely: aligned
pointer-sized writes are atomic on very nearly every processor, and that
is all that is required to remove an item from a linked list safely in
this case (though not, of course, in the general multi-threaded case).

But I can see why git wouldn't want to depend on that behavior. C11 has
a way to do this safely, but AIUI, git doesn't want to move to C99 let
alone C11.  So I guess this will just have to remain the way it is.

Re: Freeing struct lock_file?

[Torsten Bögershausen]

On 2015-04-04 02.24, David Turner wrote:
> On Fri, 2015-04-03 at 15:01 -0700, Junio C Hamano wrote:
>> David Turner <dturner@twopensource.com> writes:
>>
>>> Why is it impossible to free struct lock_files?  I understand that they
>>> become part of a linked list, and that there's an atexit handler that
>>> goes over that list.  But couldn't we just remove them from the linked
>>> list and then free them? 
>>
>> I suspect that the code is worried about getting a signal, while it
>> is manipulating the linked list, and then cause the atexit handler
>> to walk a list that is in a broken state.
> 
> This is technically possible, but practically unlikely: aligned
> pointer-sized writes are atomic on very nearly every processor, and that
> is all that is required to remove an item from a linked list safely in
> this case (though not, of course, in the general multi-threaded case).
> 
> But I can see why git wouldn't want to depend on that behavior. C11 has
> a way to do this safely, but AIUI, git doesn't want to move to C99 let
> alone C11.  So I guess this will just have to remain the way it is.
> 
If you insist on using C11, may be.

But if there is an implementation that is #ifdef'ed and only enabled for
"known to work processors" and a no-op for the others, why not ?

Do you have anything in special in mind ?
A "git diff" may be a start for a patch series..

Re: Freeing struct lock_file?

[David Turner]

On Sat, 2015-04-04 at 09:16 +0200, Torsten Bögershausen wrote:
> On 2015-04-04 02.24, David Turner wrote:
> > On Fri, 2015-04-03 at 15:01 -0700, Junio C Hamano wrote:
> >> David Turner <dturner@twopensource.com> writes:
> >>
> >>> Why is it impossible to free struct lock_files?  I understand that they
> >>> become part of a linked list, and that there's an atexit handler that
> >>> goes over that list.  But couldn't we just remove them from the linked
> >>> list and then free them? 
> >>
> >> I suspect that the code is worried about getting a signal, while it
> >> is manipulating the linked list, and then cause the atexit handler
> >> to walk a list that is in a broken state.
> > 
> > This is technically possible, but practically unlikely: aligned
> > pointer-sized writes are atomic on very nearly every processor, and that
> > is all that is required to remove an item from a linked list safely in
> > this case (though not, of course, in the general multi-threaded case).
> > 
> > But I can see why git wouldn't want to depend on that behavior. C11 has
> > a way to do this safely, but AIUI, git doesn't want to move to C99 let
> > alone C11.  So I guess this will just have to remain the way it is.
> > 
> If you insist on using C11, may be.
> 
> But if there is an implementation that is #ifdef'ed and only enabled for
> "known to work processors" and a no-op for the others, why not ?
> 
> Do you have anything in special in mind ?
> A "git diff" may be a start for a patch series..

I haven't written any code for this yet.  I wanted to understand the
current code first.

My major worry is be that the code would be somewhat fragile as it
depends on not just the processor, but also the C compiler's structure
packing rules, which are implementation-dependent.  In practice, major
compilers' rules are safe, but it's annoying to have to depend on
(especially since any bugs would be incredibly difficult to reproduce).

C99 (Was: Re: Freeing struct lock_file?)

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1091 bytes --]

On Fri, Apr 03, 2015 at 08:24:43PM -0400, David Turner wrote:
> But I can see why git wouldn't want to depend on that behavior. C11 has
> a way to do this safely, but AIUI, git doesn't want to move to C99 let
> alone C11.  So I guess this will just have to remain the way it is.

I would really like to move to at least C99.  However, MSVC bundles a
C89 compiler and a C++ compiler, but Microsoft steadfastly refuses to
bundle an actual C99 compiler, and Git doesn't even come close to
compiling as C++.  Newer versions of MSVC (2013 and newer) support
enough of the features, though, that it might be possible.

So it isn't as much of a "we don't want to move to C99" as much as "we
aren't yet willing to drop support for older versions of MSVC".  I don't
use Windows, so I'm happy to drop support for MSVC 2012 and older, but
others may have different opinions.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: C99

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> So it isn't as much of a "we don't want to move to C99" as much as "we
> aren't yet willing to drop support for older versions of MSVC".

I do not particularly like that 'we' in that sentence, which would
give a false impression to people that we all want to switch and
MSVC is the only thing that is holding us back.

Re: C99

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]

On Sat, Apr 04, 2015 at 01:06:43PM -0700, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > So it isn't as much of a "we don't want to move to C99" as much as "we
> > aren't yet willing to drop support for older versions of MSVC".
> 
> I do not particularly like that 'we' in that sentence, which would
> give a false impression to people that we all want to switch and
> MSVC is the only thing that is holding us back.

Okay, I should have clarified my statement.  I appreciate the
correction.

Some people would like to move to C99, and so far the major objections
I've heard have been that MSVC doesn't support it and that C99's
benefits are unclear.  I didn't meant to speak for others in that we
should or should not, and there might be other objections I don't recall
or haven't heard.

I seem to recall Peff wanting to use variadic macros at some point,
although I can't recall specifically where.  We also already use hacks
to implement some of the features and hope that compilers will DTRT.

All the major compilers I'm aware of other than MSVC support C99, at
least well enough to do the things we'd likely end up doing.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Freeing struct lock_file?

[David Turner]

On Fri, 2015-04-03 at 15:01 -0700, Junio C Hamano wrote:
> David Turner <dturner@twopensource.com> writes:
> 
> > Why is it impossible to free struct lock_files?  I understand that they
> > become part of a linked list, and that there's an atexit handler that
> > goes over that list.  But couldn't we just remove them from the linked
> > list and then free them? 
> 
> I suspect that the code is worried about getting a signal, while it
> is manipulating the linked list, and then cause the atexit handler
> to walk a list that is in a broken state.

Actually, couldn't we just block signals with sigprocmask while
manipulating the list?