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

[Duncan P. N. Exon Smith]

> 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

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