Re: [PATCH] fix threaded grep for machines with only one cpu

Heiko Voigt <hvoigt@hvoigt.net> writes:

An obvious question that forces reviewers to go "Huh?" is why this matters.

If use_threads is false, we do not call start_threads(), and I think at least the intent of the threaded grep code in 5b594f4 (Threaded grep, 2010-01-25) is that uninitialized mutexes matter at all in that case.

For example, "grep_mutex" is touched by calling grep_lock() and grep_unlock() macros, or pthread_* functions on it directly. There are five functions that touch this mutex: add_work(), get_work(), work_done(), start_threads(), wait_all().

 - add_work() is called from grep_sha1_async() and grep_file_async().
   They are both called _only_ when use_threads is true.

 - get_work() and work_done() are only called from run() and that is the
   function threads run from start_threads().  They cannot be reached
   unless use_threads is true.

 - wait_all() has three call sites in cmd_grep(); all of them trigger only
   when use_threads is true.

So at least for grep_mutex your patch shouldn't matter. Please explain use of which mutex is broken and how your patch fixes it.

I think the fix also assumes that an initialized mutex that is used but not destroyed is not a programming error, as wait_all() is the ONLY function that destroys these mutexes and it is called ONLY when we are using threads by calling start_threads(). But that assumption surely smells like it is sweeping a bug under a rug rather than fixing it.

Is there some programming rule I am not aware of, like "if you define a mutex or cond, you must initialize it, even if you are not going to use it at all?" If that is the case, I think your patch is necessary, but I somehow find it a bit hard to believe.