Re: [Lldb-commits] [PATCH] D38938: Logging: provide a way to safely disable logging in a forked process

[Jim Ingham via lldb-commits]

See my other message.  In a ptrace based system the inferior has to call  
PT_TRACEME to signal it should be stopped at the first instruction.  So you do 
need to run that code.  As I said, Apple added an extension to posix_spawnp to 
do this for us.

Jim


> On Oct 16, 2017, at 2:17 PM, Zachary Turner via lldb-commits 
>  wrote:
> 
> Ahh wait, sorry.  I meant posix_spawn, not execve
> 
> On Mon, Oct 16, 2017 at 2:16 PM Zachary Turner  > wrote:
> I guess what I mean is, my understanding is that the only "advantage" (if you 
> can even call it that) of using fork + exec over execve is that fork + exec 
> allows you to run additional code between the fork and the exec.
> 
> Do we actually rely on that?  Why do we need it to do fork at all?  Why can't 
> we just execve from the parent process without doing a fork at all?
> 
> On Mon, Oct 16, 2017 at 2:06 PM Tamas Berghammer  > wrote:
> On linux when you call fork the new process will only have the thread what 
> called fork. Other threads will be ignored with leaving whatever dirty state 
> they had left in the new process. Regarding execve it doesn't do fork so we 
> would have to do fork & execve what have the same issue (actually we are 
> using execve as of now but it isn't different from exec in this regard).
> 
> Tamas
> 
> On Mon, Oct 16, 2017 at 1:57 PM Zachary Turner via lldb-commits 
> mailto:lldb-commits@lists.llvm.org>> wrote:
> On Sun, Oct 15, 2017 at 3:15 PM Pavel Labath  > wrote:
> On 15 October 2017 at 23:04, Zachary Turner  > wrote:
> > Doesn’t DisableAllLogChannels acquire a scoped lock? If so wouldn’t it just
> > release at the end of the function?
> 
> 
> The thing is, it is unable to acquire the lock in the first place,
> because the mutex is already "locked". So, the sequence of events is
> process 1, thread 1: acquire lock
> process 1, thread 2: fork(), process 2 is created
> process 1 thread 1: release lock
> 
> Suppose process 1 thread 1 had been executing this code:
> ```
> lock();
> logSomething();  // thread 2 forks when thread 1 is here.
> unlock();
> ```
> 
> Doesn't thread 2 in the child process resume running from the same point?  If 
> so, it seems that both the child and parent would both gracefully unlock the 
> lock.
> 
> I'm sure I'm wrong about this, but hopefully you can clarify what I'm missing.
> 
> As a follow-up question, why can't we use execve instead of fork / exec?
> ___
> lldb-commits mailing list
> lldb-commits@lists.llvm.org 
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits 
> 
> ___
> lldb-commits mailing list
> lldb-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D38938: Logging: provide a way to safely disable logging in a forked process

[Jim Ingham via lldb-commits]

This thread behavior over fork is the same on Mach, BTW.

Apple extended posix_spawnp to take a “Stop at first instruction” attribute, 
basically just running PT_TRACEME for us.  That has ended up being very handy 
because we get all the nice cleanup behavior that pthread_spawn does, but still 
get the one thing the debugger needs fork for…  Don’t know whether it does 
something useful for locks, however.

Jim


> On Oct 16, 2017, at 2:06 PM, Tamas Berghammer via lldb-commits 
>  wrote:
> 
> On linux when you call fork the new process will only have the thread what 
> called fork. Other threads will be ignored with leaving whatever dirty state 
> they had left in the new process. Regarding execve it doesn't do fork so we 
> would have to do fork & execve what have the same issue (actually we are 
> using execve as of now but it isn't different from exec in this regard).
> 
> Tamas
> 
> On Mon, Oct 16, 2017 at 1:57 PM Zachary Turner via lldb-commits 
> mailto:lldb-commits@lists.llvm.org>> wrote:
> On Sun, Oct 15, 2017 at 3:15 PM Pavel Labath  > wrote:
> On 15 October 2017 at 23:04, Zachary Turner  > wrote:
> > Doesn’t DisableAllLogChannels acquire a scoped lock? If so wouldn’t it just
> > release at the end of the function?
> 
> 
> The thing is, it is unable to acquire the lock in the first place,
> because the mutex is already "locked". So, the sequence of events is
> process 1, thread 1: acquire lock
> process 1, thread 2: fork(), process 2 is created
> process 1 thread 1: release lock
> 
> Suppose process 1 thread 1 had been executing this code:
> ```
> lock();
> logSomething();  // thread 2 forks when thread 1 is here.
> unlock();
> ```
> 
> Doesn't thread 2 in the child process resume running from the same point?  If 
> so, it seems that both the child and parent would both gracefully unlock the 
> lock.
> 
> I'm sure I'm wrong about this, but hopefully you can clarify what I'm missing.
> 
> As a follow-up question, why can't we use execve instead of fork / exec?
> ___
> lldb-commits mailing list
> lldb-commits@lists.llvm.org 
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits 
> 
> ___
> lldb-commits mailing list
> lldb-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D38938: Logging: provide a way to safely disable logging in a forked process

[Zachary Turner via lldb-commits]

Ahh wait, sorry.  I meant posix_spawn, not execve

On Mon, Oct 16, 2017 at 2:16 PM Zachary Turner  wrote:

> I guess what I mean is, my understanding is that the only "advantage" (if
> you can even call it that) of using fork + exec over execve is that fork +
> exec allows you to run additional code between the fork and the exec.
>
> Do we actually rely on that?  Why do we need it to do fork at all?  Why
> can't we just execve from the parent process without doing a fork at all?
>
> On Mon, Oct 16, 2017 at 2:06 PM Tamas Berghammer 
> wrote:
>
>> On linux when you call fork the new process will only have the thread
>> what called fork. Other threads will be ignored with leaving whatever dirty
>> state they had left in the new process. Regarding execve it doesn't do fork
>> so we would have to do fork & execve what have the same issue (actually we
>> are using execve as of now but it isn't different from exec in this regard).
>>
>> Tamas
>>
>> On Mon, Oct 16, 2017 at 1:57 PM Zachary Turner via lldb-commits <
>> lldb-commits@lists.llvm.org> wrote:
>>
>>> On Sun, Oct 15, 2017 at 3:15 PM Pavel Labath  wrote:
>>>
 On 15 October 2017 at 23:04, Zachary Turner  wrote:
 > Doesn’t DisableAllLogChannels acquire a scoped lock? If so wouldn’t
 it just
 > release at the end of the function?


 The thing is, it is unable to acquire the lock in the first place,
 because the mutex is already "locked". So, the sequence of events is
 process 1, thread 1: acquire lock
 process 1, thread 2: fork(), process 2 is created
 process 1 thread 1: release lock

>>>
>>> Suppose process 1 thread 1 had been executing this code:
>>> ```
>>> lock();
>>> logSomething();  // thread 2 forks when thread 1 is here.
>>> unlock();
>>> ```
>>>
>>> Doesn't thread 2 in the child process resume running from the same
>>> point?  If so, it seems that both the child and parent would both
>>> gracefully unlock the lock.
>>>
>>> I'm sure I'm wrong about this, but hopefully you can clarify what I'm
>>> missing.
>>>
>>> As a follow-up question, why can't we use execve instead of fork / exec?
>>>
>> ___
>>> lldb-commits mailing list
>>> lldb-commits@lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>>>
>>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D38938: Logging: provide a way to safely disable logging in a forked process

[Zachary Turner via lldb-commits]

I guess what I mean is, my understanding is that the only "advantage" (if
you can even call it that) of using fork + exec over execve is that fork +
exec allows you to run additional code between the fork and the exec.

Do we actually rely on that?  Why do we need it to do fork at all?  Why
can't we just execve from the parent process without doing a fork at all?

On Mon, Oct 16, 2017 at 2:06 PM Tamas Berghammer 
wrote:

> On linux when you call fork the new process will only have the thread what
> called fork. Other threads will be ignored with leaving whatever dirty
> state they had left in the new process. Regarding execve it doesn't do fork
> so we would have to do fork & execve what have the same issue (actually we
> are using execve as of now but it isn't different from exec in this regard).
>
> Tamas
>
> On Mon, Oct 16, 2017 at 1:57 PM Zachary Turner via lldb-commits <
> lldb-commits@lists.llvm.org> wrote:
>
>> On Sun, Oct 15, 2017 at 3:15 PM Pavel Labath  wrote:
>>
>>> On 15 October 2017 at 23:04, Zachary Turner  wrote:
>>> > Doesn’t DisableAllLogChannels acquire a scoped lock? If so wouldn’t it
>>> just
>>> > release at the end of the function?
>>>
>>>
>>> The thing is, it is unable to acquire the lock in the first place,
>>> because the mutex is already "locked". So, the sequence of events is
>>> process 1, thread 1: acquire lock
>>> process 1, thread 2: fork(), process 2 is created
>>> process 1 thread 1: release lock
>>>
>>
>> Suppose process 1 thread 1 had been executing this code:
>> ```
>> lock();
>> logSomething();  // thread 2 forks when thread 1 is here.
>> unlock();
>> ```
>>
>> Doesn't thread 2 in the child process resume running from the same
>> point?  If so, it seems that both the child and parent would both
>> gracefully unlock the lock.
>>
>> I'm sure I'm wrong about this, but hopefully you can clarify what I'm
>> missing.
>>
>> As a follow-up question, why can't we use execve instead of fork / exec?
>>
> ___
>> lldb-commits mailing list
>> lldb-commits@lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D38938: Logging: provide a way to safely disable logging in a forked process

[Tamas Berghammer via lldb-commits]

On linux when you call fork the new process will only have the thread what
called fork. Other threads will be ignored with leaving whatever dirty
state they had left in the new process. Regarding execve it doesn't do fork
so we would have to do fork & execve what have the same issue (actually we
are using execve as of now but it isn't different from exec in this regard).

Tamas

On Mon, Oct 16, 2017 at 1:57 PM Zachary Turner via lldb-commits <
lldb-commits@lists.llvm.org> wrote:

> On Sun, Oct 15, 2017 at 3:15 PM Pavel Labath  wrote:
>
>> On 15 October 2017 at 23:04, Zachary Turner  wrote:
>> > Doesn’t DisableAllLogChannels acquire a scoped lock? If so wouldn’t it
>> just
>> > release at the end of the function?
>>
>>
>> The thing is, it is unable to acquire the lock in the first place,
>> because the mutex is already "locked". So, the sequence of events is
>> process 1, thread 1: acquire lock
>> process 1, thread 2: fork(), process 2 is created
>> process 1 thread 1: release lock
>>
>
> Suppose process 1 thread 1 had been executing this code:
> ```
> lock();
> logSomething();  // thread 2 forks when thread 1 is here.
> unlock();
> ```
>
> Doesn't thread 2 in the child process resume running from the same point?
> If so, it seems that both the child and parent would both gracefully unlock
> the lock.
>
> I'm sure I'm wrong about this, but hopefully you can clarify what I'm
> missing.
>
> As a follow-up question, why can't we use execve instead of fork / exec?
> ___
> lldb-commits mailing list
> lldb-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D38938: Logging: provide a way to safely disable logging in a forked process

[Zachary Turner via lldb-commits]

On Sun, Oct 15, 2017 at 3:15 PM Pavel Labath  wrote:

> On 15 October 2017 at 23:04, Zachary Turner  wrote:
> > Doesn’t DisableAllLogChannels acquire a scoped lock? If so wouldn’t it
> just
> > release at the end of the function?
>
>
> The thing is, it is unable to acquire the lock in the first place,
> because the mutex is already "locked". So, the sequence of events is
> process 1, thread 1: acquire lock
> process 1, thread 2: fork(), process 2 is created
> process 1 thread 1: release lock
>

Suppose process 1 thread 1 had been executing this code:
```
lock();
logSomething();  // thread 2 forks when thread 1 is here.
unlock();
```

Doesn't thread 2 in the child process resume running from the same point?
If so, it seems that both the child and parent would both gracefully unlock
the lock.

I'm sure I'm wrong about this, but hopefully you can clarify what I'm
missing.

As a follow-up question, why can't we use execve instead of fork / exec?
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D38938: Logging: provide a way to safely disable logging in a forked process

[Pavel Labath via lldb-commits]

On 15 October 2017 at 23:59, Zachary Turner  wrote:
> what are we using fork for, out of curiosity?  It seems pretty hard to make
> this work.  Logging isn’t the only thing in the program that might be
> holding a mutex, so it seems like this could still happen.
>
> Seems like mixing fork and mutexes is just a fundamentally bad idea, unless
> I’m missing something (which i very well could be)

We are using fork() (+exec()) to start a new process, which is kinda
hard to avoid, since we're in the business of running processes. :)

In general, mixing fork and anything is a bad idea and code that runs
between fork() and exec() needs to be written with a very steady hand.
(there usually isn't that much of it -- our function is 100 lines
long). Generally, one tries to use purely c library functions here,
but we also use some low level lldb functions, mainly for decoding
ProcessLaunchInfo struct. Some of them probably do some logging (I
haven't looked at which ones, but this log disabling code has been
there for ages).

Normally, it is expected that the author of this code will make sure
that all code he uses is safe to call in this funky state. However,
logging is a bit special in that in that logging calls can crop up
anywhere and we expect them to be safe to use from any context. So
even if we made sure these functions do no logging that, that can
easily change in the future.

PS: One way we could encapsulate this better would be to use the
pthread_atfork() call to register callback. That way, all of this
nastiness could be hidden in the Log class. (However, I don't think
this is particularly bad either, as ProcessLauncher is one of the
lowest layers anyway)

>
> On Sun, Oct 15, 2017 at 3:15 PM Pavel Labath  wrote:
>>
>> On 15 October 2017 at 23:04, Zachary Turner  wrote:
>> > Doesn’t DisableAllLogChannels acquire a scoped lock? If so wouldn’t it
>> > just
>> > release at the end of the function?
>>
>>
>> The thing is, it is unable to acquire the lock in the first place,
>> because the mutex is already "locked". So, the sequence of events is
>> process 1, thread 1: acquire lock
>> process 1, thread 2: fork(), process 2 is created
>> process 1 thread 1: release lock
>> everything goes well from now on in process 1...
>> process 2, thread one (and only). acquire lock:
>> BANG.
>> Process 2 is deadlocked because there is noone to release the lock there.

On 15 October 2017 at 23:59, Zachary Turner  wrote:
> what are we using fork for, out of curiosity?  It seems pretty hard to make
> this work.  Logging isn’t the only thing in the program that might be
> holding a mutex, so it seems like this could still happen.
>
> Seems like mixing fork and mutexes is just a fundamentally bad idea, unless
> I’m missing something (which i very well could be)
>
> On Sun, Oct 15, 2017 at 3:15 PM Pavel Labath  wrote:
>>
>> On 15 October 2017 at 23:04, Zachary Turner  wrote:
>> > Doesn’t DisableAllLogChannels acquire a scoped lock? If so wouldn’t it
>> > just
>> > release at the end of the function?
>>
>>
>> The thing is, it is unable to acquire the lock in the first place,
>> because the mutex is already "locked". So, the sequence of events is
>> process 1, thread 1: acquire lock
>> process 1, thread 2: fork(), process 2 is created
>> process 1 thread 1: release lock
>> everything goes well from now on in process 1...
>> process 2, thread one (and only). acquire lock:
>> BANG.
>> Process 2 is deadlocked because there is noone to release the lock there.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D38938: Logging: provide a way to safely disable logging in a forked process

[Zachary Turner via lldb-commits]

what are we using fork for, out of curiosity?  It seems pretty hard to make
this work.  Logging isn’t the only thing in the program that might be
holding a mutex, so it seems like this could still happen.

Seems like mixing fork and mutexes is just a fundamentally bad idea, unless
I’m missing something (which i very well could be)

On Sun, Oct 15, 2017 at 3:15 PM Pavel Labath  wrote:

> On 15 October 2017 at 23:04, Zachary Turner  wrote:
> > Doesn’t DisableAllLogChannels acquire a scoped lock? If so wouldn’t it
> just
> > release at the end of the function?
>
>
> The thing is, it is unable to acquire the lock in the first place,
> because the mutex is already "locked". So, the sequence of events is
> process 1, thread 1: acquire lock
> process 1, thread 2: fork(), process 2 is created
> process 1 thread 1: release lock
> everything goes well from now on in process 1...
> process 2, thread one (and only). acquire lock:
> BANG.
> Process 2 is deadlocked because there is noone to release the lock there.
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D38938: Logging: provide a way to safely disable logging in a forked process

[Pavel Labath via lldb-commits]

On 15 October 2017 at 23:04, Zachary Turner  wrote:
> Doesn’t DisableAllLogChannels acquire a scoped lock? If so wouldn’t it just
> release at the end of the function?


The thing is, it is unable to acquire the lock in the first place,
because the mutex is already "locked". So, the sequence of events is
process 1, thread 1: acquire lock
process 1, thread 2: fork(), process 2 is created
process 1 thread 1: release lock
everything goes well from now on in process 1...
process 2, thread one (and only). acquire lock:
BANG.
Process 2 is deadlocked because there is noone to release the lock there.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D38938: Logging: provide a way to safely disable logging in a forked process

[Zachary Turner via lldb-commits]

Doesn’t DisableAllLogChannels acquire a scoped lock? If so wouldn’t it just
release at the end of the function?
On Sun, Oct 15, 2017 at 2:42 PM Pavel Labath via Phabricator <
revi...@reviews.llvm.org> wrote:

> labath created this revision.
>
> We had a bug where if we had forked (in the ProcessLauncherPosixFork)
> while another thread was writing a log message, we would deadlock. This
> happened because the fork child inherited the locked log rwmutex, which
> would never get unlocked. This meant the child got stuck trying to
> disable all log channels.
>
> The bug existed for a while but only started being apparent after
> https://reviews.llvm.org/D37930, which started using ThreadLauncher
> (which uses logging) instead
> of std::thread (which does not) for launching TaskPool threads.
>
> The fix is to provide a lock free way to disable logging, which should
> be used in this case, and this case only.
>
>
> https://reviews.llvm.org/D38938
>
> Files:
>   include/lldb/Utility/Log.h
>   source/Host/posix/ProcessLauncherPosixFork.cpp
>   source/Utility/Log.cpp
>
>
> Index: source/Utility/Log.cpp
> ===
> --- source/Utility/Log.cpp
> +++ source/Utility/Log.cpp
> @@ -90,7 +90,10 @@
>
>  void Log::Disable(uint32_t flags) {
>llvm::sys::ScopedWriter lock(m_mutex);
> +  DisableUnlocked(flags);
> +}
>
> +void Log::DisableUnlocked(uint32_t flags) {
>uint32_t mask = m_mask.fetch_and(~flags, std::memory_order_relaxed);
>if (!(mask & ~flags)) {
>  m_stream_sp.reset();
> @@ -241,6 +244,11 @@
>  entry.second.Disable(UINT32_MAX);
>  }
>
> +void Log::DisableAllLogChannelsUnlocked() {
> +  for (auto &entry : *g_channel_map)
> +entry.second.DisableUnlocked(UINT32_MAX);
> +}
> +
>  void Log::ListAllLogChannels(llvm::raw_ostream &stream) {
>if (g_channel_map->empty()) {
>  stream << "No logging channels are currently registered.\n";
> Index: source/Host/posix/ProcessLauncherPosixFork.cpp
> ===
> --- source/Host/posix/ProcessLauncherPosixFork.cpp
> +++ source/Host/posix/ProcessLauncherPosixFork.cpp
> @@ -97,7 +97,7 @@
>const ProcessLaunchInfo
> &info) {
>// First, make sure we disable all logging. If we are logging to
> stdout, our
>// logs can be mistaken for inferior output.
> -  Log::DisableAllLogChannels();
> +  Log::DisableAllLogChannelsUnlocked();
>
>// Do not inherit setgid powers.
>if (setgid(getgid()) != 0)
> Index: include/lldb/Utility/Log.h
> ===
> --- include/lldb/Utility/Log.h
> +++ include/lldb/Utility/Log.h
> @@ -116,6 +116,11 @@
>
>static void DisableAllLogChannels();
>
> +  // Only use if no other threads are executing and calling the locking
> version
> +  // is known to be unsafe (for example, this happens right after
> forking, if we
> +  // have forked while another thread was holding the log mutex)).
> +  static void DisableAllLogChannelsUnlocked();
> +
>static void ListAllLogChannels(llvm::raw_ostream &stream);
>
>//--
> @@ -184,6 +189,7 @@
>uint32_t options, uint32_t flags);
>
>void Disable(uint32_t flags);
> +  void DisableUnlocked(uint32_t flags);
>
>typedef llvm::StringMap ChannelMap;
>static llvm::ManagedStatic g_channel_map;
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits