[PATCH] Change base mutex implementations to use STL-provided mutexes

[Zachary Turner]

It also leads to implementations being not as efficient as they could be.
 for example, it's easy to write portable code by using either the C or C++
standard libraries, but this also means you can sometimes end up with less
than optimal implementations on some or all platforms.

It also leads to situations like this one.  In particular, as I'm working
on removing windows.h from mutex.h, it turns out that the only place that
actually needs this CriticalSection is in libclang, to work around a
limitation of their implementation.  Since these .inc files are internal to
LLVM, nothing in them is exposed to libclang.  The paradigm is to make a
common interface and then provide an implementation for each platform.  In
this case, there's no equivalent on other platforms.

In other words, LLVM isn't necessarily the only library in the system that
needs to make platform-specific decisions, but none of the platform
specific implementations are exposed outside of LLVM.

The solution I'm using is to declare CriticalSectionMutex inside of Mutex.h
wrapped inside of an #if defined(LLVM_ON_WIN32) with a void* impl that will
be allocated as a Win32 CRITICAL_SECTION struct.  Still, you get the
#ifdefs in the header file, when it would be much easier and cleaner for
libclang to simply #include "llvm\Support\Windows\CriticalSectionMutex.h"


On Fri, Jun 6, 2014 at 1:42 PM, Alp Toker <alp at nuanti.com> wrote:

>
> On 06/06/2014 23:15, Zachary Turner wrote:
>
>> It's definitely possible, it's just a lot of work.  But it can be done in
>> a way that is low maintenance and actually *increases* portability and code
>> maintainability, in my opinion.  Chrome does it for example, and that is a
>> pretty large project.
>>
>
> Yep, I'm familiar with Chromium, and followed it's evolution from WebKit
> over the years. It's great you mention this because they're both fantastic
> porting layers with a one big distinction:
>
>  * Chromium's abstraction makes it *easy to write portable code* that
> integrates with the platform. Integrated by default, but porting is still a
> conscious step you have to do.
>
>  * LLVM's abstraction makes it *difficult to write code that isn't
> portable*. You don't get to touch the OS, and if you do, you have to use it
> directly instead of reusing LLVM's internal abstraction. It turns out that
> leads to people writing more trivially portable code without conscious
> effort.
>
> And these seem to be the right abstraction points for a browser and
> compiler toolchain respectively.
>
> I also agree with what Chandler said, with the caveat that LLVM has less
> centralised control compared to Chromium/WebKit, so while it might be
> technically elegant to expose a "middle layer" of platform abstractions,
> there's a higher risk that someone will break that the week you're on
> vacation on a decentralised project like LLVM. That's why the .inc files
> make a brilliant -- if slightly quirky -- point of abstraction for a
> compiler toolchain. I think it's hands down one of the strongest areas of
> this project so we'd do well to keep with the program :-)
>
> Alp.
>
>
>
>  Either way, it's independent of this particular change, so I will fight
>> that battle another time :)
>>
>>
>> On Fri, Jun 6, 2014 at 1:10 PM, Chandler Carruth <chandlerc at gmail.com
>> <mailto:chandlerc at gmail.com>> wrote:
>>
>>
>>     On Fri, Jun 6, 2014 at 12:32 PM, Alp Toker <alp at nuanti.com
>>     <mailto:alp at nuanti.com>> wrote:
>>
>>         Hi Zachary,
>>
>>         In LLVM platform-specific code goes in the implementation
>>         files (.inc) and never in the portable headers. Certainly for
>>         a high-level class like Mutex I wouldn't expect to see
>>         platform-specific conditionals exposed.
>>
>>         A couple of exceptions have slipped in like
>>         Support/FileSystem.h and Support/Process.h with ifdef WIN32
>>         but those are meant to get fixed at some point.
>>
>>
>>     While you may be describing the current situation, I don't think
>>     that this is really a strong rule than many LLVM developers feel
>>     constrained by, and I wouldn't want it to be.
>>
>>     I think the important rule is subtly but importantly different:
>>     the header files must, ultimately, be portable. Where possible, it
>>     is useful to isolate platform specific code away from header
>>     files, but it is not always possible or desirable. Either way, the
>>     header file needs to carefully manage any platform specifics
>>     involved to ensure it works correctly and reliably across platforms.
>>
>>     One example of this (but not necessarily a *good* example -- I
>>     would use a different technique) is our DataTypes.h which is
>>     generated at configure/cmake time.
>>
>>     I think it is entirely possible that the correct thing to do for
>>     some parts of Support is to include windows.h with appropriate
>>     control of macros and behind appropriate guards. I say it is
>>     "possible" because I don't know very much about the *specific*
>>     case of windows.h and its problems. Perhaps there are specific
>>     problems with it that make it intractable to use in this way even
>>     on Windows. But the general concept seems fine when useful.
>>     Similarly, if we wanted to include a Mac OS X header file to
>>     access a synchronization or other primitive provided by the OS, it
>>     would seem reasonable to include that header, again provided
>>     sufficient care and guards are used to make sure the header, as a
>>     whole, remains a reliable header for other projects to use on each
>>     platform.
>>
>>
>>
> --
> http://www.nuanti.com
> the browser experts
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140606/edce9944/attachment.html>