On Fri, Jan 13, 2012 at 10:24 PM, Chris Lattner <clattner at apple.com> wrote:> > On Jan 13, 2012, at 4:43 PM, David Blaikie wrote: > >> It looks like for a while now (5 years) some code that was meant to do >> smart things in Mutex.cpp hasn't been doing such smart things: >> >> https://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/Mutex.cpp?r1=29287&r2=29932&diff_format=h >> >> I don't know if this change was even deliberate since it came in with >> another change & the description doesn't mention it. Should we do away >> with this logic if it hasn't been needed all this time? Reinstate it >> (with the old logic? or with a configure-based preprocessor variable >> instead)? > > On some (linux?) implementations, various pthread APIs are defined as "weak extern" symbols in libc and strong definitions in libpthreads. The idea of this check is thus to detect if pthreads is linked into the app and enable threads if so.Sorry, right - I should've been more clear. I understand that that's what the declaration/boolean used to do: static const bool pthread_enabled = static_cast<bool>(pthread_mutex_init); But that's not what the code does now - the change you made 5 years ago changed it to a constant: static const bool pthread_enabled = true; (I found this because the return outside the 'if (pthread_unabled) { ... return }' was marked as unreachable by -Wunreachable-code). Since it hasn't actually been doing anything for 5 years - should we just remove the variable & all the conditions (unconditionally using pthreads), or reinstate the smarts it used to have (testing the weak symbol)?
On Jan 13, 2012, at 11:17 PM, David Blaikie wrote:>> On some (linux?) implementations, various pthread APIs are defined as "weak extern" symbols in libc and strong definitions in libpthreads. The idea of this check is thus to detect if pthreads is linked into the app and enable threads if so. > > Sorry, right - I should've been more clear. I understand that that's > what the declaration/boolean used to do: > > > static const bool pthread_enabled = static_cast<bool>(pthread_mutex_init); > > But that's not what the code does now - the change you made 5 years > ago changed it to a constant: > > static const bool pthread_enabled = true; > > (I found this because the return outside the 'if (pthread_unabled) { > ... return }' was marked as unreachable by -Wunreachable-code). Since > it hasn't actually been doing anything for 5 years - should we just > remove the variable & all the conditions (unconditionally using > pthreads), or reinstate the smarts it used to have (testing the weak > symbol)?Aha, I'm completely fine with constant folding this all away. Simple is good. -Chris
On Fri, Jan 13, 2012 at 11:58 PM, Chris Lattner <clattner at apple.com> wrote:> On Jan 13, 2012, at 11:17 PM, David Blaikie wrote: > >>> On some (linux?) implementations, various pthread APIs are defined as "weak extern" symbols in libc and strong definitions in libpthreads. The idea of this check is thus to detect if pthreads is linked into the app and enable threads if so. >> >> Sorry, right - I should've been more clear. I understand that that's >> what the declaration/boolean used to do: >> >> >> static const bool pthread_enabled = static_cast<bool>(pthread_mutex_init); >> >> But that's not what the code does now - the change you made 5 years >> ago changed it to a constant: >> >> static const bool pthread_enabled = true; >> >> (I found this because the return outside the 'if (pthread_unabled) { >> ... return }' was marked as unreachable by -Wunreachable-code). Since >> it hasn't actually been doing anything for 5 years - should we just >> remove the variable & all the conditions (unconditionally using >> pthreads), or reinstate the smarts it used to have (testing the weak >> symbol)? > > Aha, I'm completely fine with constant folding this all away. Simple is good.Sounds good. Committed as r148206 (including a similar fix in RWMutex.cpp - which propagated the same quirk when it was copy/pasted/created from Mutex.cpp) I've attached a patch containing some other fixes to remove unreachable code in LLVM - this isn't comprehensive, but a start based on -Wunreachable-code. Is this sort of thing OK to commit as I work through it? About the most noteworthy parts are: 1) two instances of code deliberately if(false)ed out with comments explaining that developers can bring it back in once something works or to aid in debugging. I switched these to #if 0 instead - I realize this means those code snippets may bitrot since they won't be compiled anymore. Is this the right thing? Should we have some construct for "deliberately unreachable" code like this? (perhaps like the parentheses warning - some quirky syntax people could use when they intend to create an always false condition) 2) Mostly it was just replacing trailing returns that used to be there to satisfy GCC with llvm_unreachable instead. (also there were a lot of break/return/etc after llvm_Unreachables which were easily removed) - David -------------- next part -------------- A non-text attachment was scrubbed... Name: llvm_unreachable_fixes.diff Type: application/octet-stream Size: 6129 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20120114/0c262c4e/attachment.obj>