CoreFX native PAL build break on FreeBSD due to "error: mutex 'lock' is not held on every path"

[wfurt]

Attempt to build native parts on FreeBSD no longer works. This seems to be related to dotnet/corefx#26291

[furt@FreeBSD-11 ~/git/corefx]$ ./src/Native/build-native.sh -clang3.9
...
[ 44%] Building C object System.Native/CMakeFiles/System.Native-Static.dir/pal_signal.c.o
/usr/home/furt/git/corefx/src/Native/Unix/System.Native/pal_signal.c:151:17: error: mutex 'lock' is not held on every path through here [-Werror,-Wthread-safety-analysis]
if (callback != NULL)
^
/usr/home/furt/git/corefx/src/Native/Unix/System.Native/pal_signal.c:136:17: note: mutex acquired here
pthread_mutex_lock(&lock);
^
/usr/home/furt/git/corefx/src/Native/Unix/System.Native/pal_signal.c:85:1: note: Thread warning in function 'SignalHandlerLoop'
{

by quick looking at the code, there seems to be unbalanced pthread_mutex_lock()
Following change fixes the issue but I'm not sure if that was the intention:

--- a/src/Native/Unix/System.Native/pal_signal.c
+++ b/src/Native/Unix/System.Native/pal_signal.c
@@ -146,6 +146,7 @@ static void* SignalHandlerLoop(void* arg)
                         } while (pid > 0);
                     }
                 }
+                pthread_mutex_unlock(&lock);
             }

             if (callback != NULL)

cc: @tmds @janvorli @mateusrodrigues

[tmds]

@wfurt, indeed there is a missing unlock!
Fortunately, it is in the case where SIGCHLD is set to SIG_IGN, which is very uncommon.

Really nice that the compiler detected that. Did we change some flags for this? Or is clang smarter when it runs on FreeBSD?

Will you do a PR to fix the issue? Or should I?

[wfurt]

either way is fine. And it is the tool - not me :)
I did not try to run FreeBSD build for a while.

[janvorli]

Or is clang smarter when it runs on FreeBSD?

I've noticed that in the past too - clang on FreeBSD was reporting issues that the same version of clang on Linux was not. Not sure what causes that, maybe default state of some compiler options is different.

[stephentoub]

Reopening to track 2.1 port of fix.

[danmoseley]

@wfurt can you please determine whether it is possible to workaround this (without the fix obviously) by passing a flag to the compiler to disable this diagnostic? I assume this is possible.

This is a request from shiproom to help determine whether we should backport to release/2.1

(Or instead backport disabling the diagnostic)

[wfurt]

yes, we can workaround it for FreeBSD. There are other FreeBSD issues for 2.1 branches so it is not blocking. We should consider the fix for other reasons.

[tmds]

@danmosemsft this is ported to 2.1 here: dotnet/corefx#29845

[stephentoub]

@tmds, but not merged; Dan is gathering data to determine whether it needs to be.