[llvm] r209227 - GlobalValue: Automatically reset visibility when setting local linkage

[Duncan P. N. Exon Smith]

> On 2014-Jul-24, at 13:36, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
>> On 2014-Jul-24, at 11:37, Juergen Ributzka <juergen at apple.com> wrote:
>> 
>> Hi Duncan,
>> 
>> sorry for catching this so late. As it turns out, clang is not the only client that sets the visibility before the linkage. These methods are exposed via the C API and I didn’t see any documentation that there is an ordering constrained on these two methods. Since the C API has to be backward compatible it is possible that these recent changes can break existing clients like WebKit.
>> 
>> -Juergen
> 
> What exactly is the problem?
> 
> Is the *other* assertion added in r208264 (in `setVisibility()`) firing?
> 
> Or is the logic from r209227 in `setLinkage()` doing the wrong thing?
> If so, how is it wrong?
> 
> For context, this was reviewed in [1][2] and committed in r207978,
> r207979, r208261, r208258, r208262, r208263, and r208264.
> 
> [1]: http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140428/215510.html
> [2]: http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140505/215785.html

*ping*

Can you confirm what the problem is?  It'd be better to solve it *before*
releasing 3.5.

>> On May 20, 2014, at 12:00 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>> 
>>> Author: dexonsmith
>>> Date: Tue May 20 14:00:58 2014
>>> New Revision: 209227
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=209227&view=rev
>>> Log:
>>> GlobalValue: Automatically reset visibility when setting local linkage
>>> 
>>> r208264 started asserting in `setLinkage()` and `setVisibility()` that
>>> visibility and linkage are compatible.  There are a few places in clang
>>> where visibility is set first, and then linkage later, so the assert
>>> fires.  In `setLinkage()`, it's clear what the visibility *should* be,
>>> so rather than updating all the call sites just automatically fix the
>>> visibility.
>>> 
>>> The testcase for this is for *clang*, so it'll follow separately in cfe.
>>> 
>>> PR19760
>>> 
>>> Modified:
>>>  llvm/trunk/include/llvm/IR/GlobalValue.h
>>> 
>>> Modified: llvm/trunk/include/llvm/IR/GlobalValue.h
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/GlobalValue.h?rev=209227&r1=209226&r2=209227&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/include/llvm/IR/GlobalValue.h (original)
>>> +++ llvm/trunk/include/llvm/IR/GlobalValue.h Tue May 20 14:00:58 2014
>>> @@ -222,8 +222,8 @@ public:
>>> bool hasCommonLinkage() const { return isCommonLinkage(Linkage); }
>>> 
>>> void setLinkage(LinkageTypes LT) {
>>> -    assert((!isLocalLinkage(LT) || hasDefaultVisibility()) &&
>>> -           "local linkage requires default visibility");
>>> +    if (isLocalLinkage(LT))
>>> +      Visibility = DefaultVisibility;
>>>   Linkage = LT;
>>> }
>>> LinkageTypes getLinkage() const { return Linkage; }
>>> 
>>> 
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
>