[patch] [libcxx] _LIBCPP_WEAK

[Reid Kleckner]

On Wed, Sep 25, 2013 at 4:48 PM, G M <gmisocpp at gmail.com> wrote:

> On Thu, Sep 26, 2013 at 7:18 AM, Reid Kleckner <rnk at google.com> wrote:
>
>> I'm OK with this because it helps libc++ build, but I can't yet see a
>> path forward for actually implementing replaceable operator new on Windows.
>>
>> I agree that you can drop the visibility("default") bits from new.cpp,
>> because it's already in the header.  I would probably duplicate
>> _LIBCPP_NEW_DELETE_VIS on the definitions in new.cpp for consistency and
>> documentation, since the visibility of new and delete is one of the most
>> interesting things about it.
>>
> Just to be clear here, you're agreeing I "can drop" the
> visibility("default") (implied: in theory?) , but that I "shouldn't" for
> documentation purposes (in practice.). Is that correct?
>
> Second, I'm ok to do that, _LIBCPP_NEW_DELETE_VIS expands "default" as
> does _LIBCPP_FUNC_VIS.
>
> but we don't put "default" attributes on definitions anywhere else in
> libcxx, it'd be quite irregular. So why here and not there?
>
> Unless _LIBCPP_NEW_DELETE_VIS expands to something other than
> _LIBCPP_FUNC_VIS in some situations that I've missed?
>
> If _LIBCPP_NEW_DELETE_VIS always does expand to _LIBCPP_FUNC_VIS, do we
> need the former?
>
> And if we're going to put _LIBCPP_X_VIS attributes on definitions here,
> shouldn't we also be putting them on definitions everywhere else? But that
> would be a big task.
>
> I kind of agree it does help documentation, but it seems we are either
> inconsistent if we do as you suggest, and if we fix that inconsistency we
> have a big job to do to do that everywhere.
>
> So with that all said? Could you double confirm what you think here?
>

I don't actually feel strongly about this.  It's up to Howard.


> The changes to __config to detect RTTI and implement _LIBCPP_WARNING are
>> unrelated and shouldn't be in this patch.
>>
>
> It's true, it's just I'm hoping the other patches will be applied prior
> and this would make merging easier. It certainly made my life easier. The
> warning patch, you've LGTM'd I think, I only have a problem here if the
> rtti patch doesn't get LGTM'd. Is that correct?
>

I stamped it, FWIW.  It fell out of my inbox.


> Which is a good time to ask, are you ok with the RTTI patch. It fixes
> ninga to build "out of the box" amongst other things. Without it, the
> libcpp was failing to recognize -rno-rtti option in some cases.
>
> Thanks
>
>
>>
>>
>> On Wed, Sep 25, 2013 at 6:42 AM, G M <gmisocpp at gmail.com> wrote:
>>
>>> Hi Everyone
>>>
>>> The attached patch is for libcxx's new.cpp and __config files. The
>>> patch's intent is to make new.cpp compile using MS's cl.exe compiler
>>> without changing the meaning of anything for any other compiler.
>>>
>>> The issue this patch seeks to address is that MS's compiler (cl.exe)
>>> doesn't support the __attribute__((__weak__)) or
>>> __atribute__((__visibility__("default")) syntax; so a solution must be
>>> found where cl.exe doesn't see this syntax.
>>>
>>> This patch seeks to solve this problem by changing code patterned like
>>> this:
>>> __attribute__((__weak__, __visibility__("default")))
>>> void* operator new(size_t size, const std::nothrow_t&) _NOEXCEPT {
>>> /*snip*/; return p; }
>>>
>>> to code like this:
>>> _LIBCPP_WEAK
>>> void* operator new(size_t size, const std::nothrow_t&) _NOEXCEPT {
>>> return p; }
>>>
>>> with the expectation that this change will NOT introduce any
>>> functionality change for clang++/g++ etc. That expectation is based on two
>>> aspects of the change:
>>> * The first is the belief that cl.exe doesn't support "weak" in any
>>> documented way and that libcxx on Windows doesn't need it
>>> anyway. So _LIBCPP_WEAK is defined as nothing when cl.exe is the detected
>>> compiler.
>>>
>>> For all other compilers, _LIBCPP_WEAK is defined to be just
>>> __attribute__((__weak__)) and nothing more).
>>>
>>> This should mean that cl.exe doesn't see the weak attribute syntax and
>>> so won't choke on it; and g++/clang++ will see the same weak attribute that
>>> it saw before this patch.
>>> * The second part is what to do about
>>> __attribute__((_visibility__("default"))) as in the proposed change it is
>>> dropped from the function definition.
>>>
>>> The expecatation here is that this is ok because it isn't neccessary
>>> because the prototype for the modified functions already have it; so the
>>> right thing should still happen.
>>> If all of this is correct, then this patch should fix new.cpp for cl.exe
>>> without changing anything else.
>>>
>>> It also provides a pattern that will work with all the compilers libcxx
>>> already supports; and without having to introduce alternate #if/#else
>>> guards or other uglyness. This should make it better match the patterns
>>> libcxx already uses.
>>>
>>> If removing the "default" attribute turns out to be a problem, I
>>> believe the default attribute could be added back now that it is decoupled
>>> from the "weak" attribute (which I think is a good thing in of itself) by
>>> using one of libcxx's existing macro's such as _LIBCPP_FUNC_VIS /
>>> _LIBCPP_NEW_DELETE_VIS etc.
>>>
>>> I'm not sure of the neccessity of LIBCPP_NEW_DELETE_VIS or it's
>>> realtionship to _LIBCPP_FUNC_VIS at this point, FWIW, but that doesn't
>>> matter to the logic of this patch.
>>>
>>> I compiled this patch with cl.exe, g++ and clang++.exe.
>>> Please let me know what you think. If this patch doesn't get traction,
>>> I'd appreciate some advice with real alternative code that could be used to
>>> advance things here as I found it hard to produce something actionable from
>>> the comments I received to my previous patch for this problem though I did
>>> and do appreciate the responses.
>>>
>>> Thanks
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130925/70371f4e/attachment.html>