[llvm] r226490 - IR: Remove direct comparisons against Metadata::Storage, NFC

Tue Jan 20 07:59:05 PST 2015

Duncan just committed this as r226570.

From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Timur Iskhodzhanov
Sent: Tuesday, January 20, 2015 17:54
To: Duncan P. N. Exon Smith
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: [llvm] r226490 - IR: Remove direct comparisons against Metadata::Storage, NFC

This is what I get on my Windows machine:
  $ cat test.cpp
  char hello;
  $ bin\clang.exe -cc1 -gline-tables-only -triple i686-pc-linux -emit-obj -o z test.cpp
  Assertion failed: DbgNode->isTemporary() && "Expected temporary node", file ..\lib\IR\DebugInfo.cpp, line 350
but on Linux it's fine.

Replacing
  StorageType Storage : 2;
with
  unsigned Storage : 2;
seems to resolve the issue.
Shall I commit this change?

Reid might know the enum stuff better.

On Tue Jan 20 2015 at 6:39:22 PM Duncan P. N. Exon Smith <dexonsmith at apple.com<mailto:dexonsmith at apple.com>> wrote:

> On 2015 Jan 20, at 07:32, Timur Iskhodzhanov <timurrrr at google.com<mailto:timurrrr at google.com>> wrote:
>
>
>
> On Tue Jan 20 2015 at 6:20:34 PM Duncan Exon Smith <dexonsmith at apple.com<mailto:dexonsmith at apple.com>> wrote:
>
>
> On Jan 20, 2015, at 3:48 AM, Timur Iskhodzhanov <timurrrr at google.com<mailto:timurrrr at google.com>> wrote:
>
>> Hi Duncan,
>>
>> This has broken down "ninja check-asan" on 32-bit Windows.
>> Assertion failed: Context.hasReplaceableUses() && "Expected RAUW support", file C:\src\llvm_bot\slave\win\build\llvm\include\llvm/IR/Metadata.h, line 976
>
> Can you include the asan output?   A backtrace?   Point at a buildbot?   I don't have the tools to reproduce this.
>
> There's no public buildbot for ASan itself, but turns out this problem is not even specific to ASan.
>
> This public bot just builds Chromium with Clang
> http://build.chromium.org/p/chromium.fyi/builders/CrWinClang/builds/1295/steps/compile/logs/stdio
>
> Assertion failed: isTemporary() && "Expected temporary node", file C:\b\build\slave\CrWinClang\build\src\third_party\llvm\include\llvm/IR/Metadata.h, line 741
> since r226499.
>
> OK, I'll try to minimize...
> (I suppose the change looks sane at the first glance and you can't come up with a repro yourself)
>
> It seems to be hard to revert now...
>> You've committed a lot of small changes in a very short timeframe -
>
> Actually, I committed work over the span of an entire work day.
>
> Oh, I see.
>> - I assume you're using git?  Is there a reason why you don't squash your changes before committing?  It makes finding bad commits / reverting harder...
>
> Besides being developer policy to separate changes as much as possible, it also makes changes easier to review, and isolating which change causes a problem easier to determine.
>
> Unless it results in complex history and back-in-forth changes, I suppose.
> I've spent more than an hour today trying to locally bisect and revert the changes and ended up just locally reverting 40+ of your commits after 226490 to make progress.  Unfortunately, reverting just r226490 results in a waterfall of merge conflicts.

Yup, all the others depend on it.  I would have dealt with it myself
if a buildbot had told me about it.

> Do you think it's reasonable to temporarily revert your changes in the [r226490, HEAD] range to unbreak stuff?
> I have a patch ready for that.

Certainly an option, although I'm actively looking at this right now
(sorry for the delay, I'm in PDT so I just woke up).

Did you do a full bisect?  Are you sure that leaving this in and
reverting the ones that follow don't also fix it?

Do you know if MSVC handles things like:

    enum StorageType { ... };
    StorageType Storage : 2;

correctly (well, the way I expect), or should I store it as an

    unsigned Storage : 2;

for compatibility?  Any chance it causes a weird layout problem that
this commit happens to point out?

(Given that this is MSVC-only, I suspect there's a layout problem or
UB somewhere.)

>> On Mon Jan 19 2015 at 10:30:36 PM Duncan P. N. Exon Smith <dexonsmith at apple.com<mailto:dexonsmith at apple.com>> wrote:
>> Author: dexonsmith
>> Date: Mon Jan 19 13:26:24 2015
>> New Revision: 226490
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=226490&view=rev
>> Log:
>> IR: Remove direct comparisons against Metadata::Storage, NFC
>>
>> Modified:
>>     llvm/trunk/lib/IR/Metadata.cpp
>>
>> Modified: llvm/trunk/lib/IR/Metadata.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Metadata.cpp?rev=226490&r1=226489&r2=226490&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/IR/Metadata.cpp (original)
>> +++ llvm/trunk/lib/IR/Metadata.cpp Mon Jan 19 13:26:24 2015
>> @@ -402,7 +402,7 @@ MDNode::MDNode(LLVMContext &Context, uns
>>    for (unsigned I = 0, E = MDs.size(); I != E; ++I)
>>      setOperand(I, MDs[I]);
>>
>> -  if (Storage == Temporary)
>> +  if (isTemporary())
>>      this->Context.makeReplaceable(
>>          make_unique<ReplaceableMetadataImpl>(Context));
>>  }
>> @@ -416,7 +416,7 @@ static bool isOperandUnresolved(Metadata
>>  UniquableMDNode::UniquableMDNode(LLVMContext &C, unsigned ID,
>>                                   StorageType Storage, ArrayRef<Metadata *> Vals)
>>      : MDNode(C, ID, Storage, Vals) {
>> -  if (Storage != Uniqued)
>> +  if (!isUniqued())
>>      return;
>>
>>    // Check whether any operands are unresolved, requiring re-uniquing.
>> @@ -432,7 +432,7 @@ UniquableMDNode::UniquableMDNode(LLVMCon
>>  }
>>
>>  void UniquableMDNode::resolve() {
>> -  assert(Storage == Uniqued && "Expected this to be uniqued");
>> +  assert(isUniqued() && "Expected this to be uniqued");
>>    assert(!isResolved() && "Expected this to be unresolved");
>>
>>    // Move the map, so that this immediately looks resolved.
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150120/d487146f/attachment.html>