[patch] [libcxx] _LIBCPP_WEAK

[G M]

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?



> 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?

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/20130926/b3f0336b/attachment.html>