New issue
Advanced search Search tips

Issue 749813 link

Starred by 4 users
Status: Fixed
Owner:
r...@chromium.org
Closed: Aug 2017
Cc:
r...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 3
Type: Bug

Blocked on:
issue 750434
Blocking:
issue 636111



Sign in to add a comment

Clang Windows symbols attributes object pointer loads to contructor line

Project Member Reported by brettw@chromium.org, Jul 27 2017 
I was stepping through some code in Visuyal Studio 2017 generated by Clang and the current line would jump around randomly like I was debugging a release build.

For a concrete example, see //base/values_unittest.cc step through TEST(ValuesTest, MoveList) and the current line will jump to near the top of the function several times. I was debugging some more complicated things and it was much worse.

Looking at the assembly for:

  const Value::ListStorage list = {Value(123)};
  Value value(list);
  Value moved_value(std::move(value));
  EXPECT_EQ(Value::Type::LIST, moved_value.type());
  EXPECT_EQ(123, moved_value.GetList().back().GetInt());

it's more obvious what's going on:

Value value(list);   <========
   mov         r8,qword ptr [rsp+138h]  
   mov         qword ptr [rsp+130h],rax  
   call        r8  
   lea         rcx,[value]  
Value moved_value(std::move(value));
   mov         qword ptr [rsp+128h],rax  
   ...
   lea         rcx,[moved_value]  
EXPECT_EQ(Value::Type::LIST, moved_value.type());
   mov         rdx,qword ptr [__imp_base::Value::type (0141A46790h)]  
   ...
   jmp         base::ValuesTest_MoveList_Test::TestBody+2CCh (01412E0A7Ch)  
Value value(list);   <========
   lea         rcx,[rsp+218h]  
EXPECT_EQ(Value::Type::LIST, moved_value.type());
   call        testing::Message::Message (014004774Eh)  
   ...
   call        testing::Message::~Message (0140002513h)  
Value value(list);   <========
   lea         rcx,[rsp+228h]  
EXPECT_EQ(Value::Type::LIST, moved_value.type());
   call        testing::AssertionResult::~AssertionResult (014001380Eh)  
   lea         rcx,[moved_value]  
EXPECT_EQ(123, moved_value.GetList().back().GetInt());
   mov         rax,qword ptr [__imp_base::Value::GetList (0141A467F8h)]  
   ...
   jmp         base::ValuesTest_MoveList_Test::TestBody+3FFh (01412E0BAFh)  
Value value(list);   <========
   lea         rcx,[rsp+1F0h]  
EXPECT_EQ(123, moved_value.GetList().back().GetInt());
   call        testing::Message::Message (014004774Eh)  
   ...
   call        testing::Message::~Message (0140002513h)  
Value value(list);   <========
   lea         rcx,[rsp+200h]  
EXPECT_EQ(123, moved_value.GetList().back().GetInt());
   call        testing::AssertionResult::~AssertionResult (014001380Eh)  
   ...

It keeps attributing rcx loads to the "Value value(list)" constructor. It looks like this is loading the value pointer into a register as the "this" value to call a function, and that load is getting attributed to the constructor of the object being loaded.

Loading the pointer for a function call should be attributed to the call line instead.

Comment 1 by thakis@chromium.org, Jul 27 2017

Blockedon: -636111
Blocking: 636111
Cc: r...@chromium.org
Labels: OS-Windows

Comment 2 by r...@chromium.org, Jul 28 2017 

The assembler .cv_loc directives clang is emitting seem to make sense, they ascend for this straight-line code:

$ grep 'cv_loc\s134\s' values_unittest.s
        .cv_loc 134 1 347 0             # ../../base/values_unittest.cc:347:0
        .cv_loc 134 1 348 0             # ../../base/values_unittest.cc:348:0
        .cv_loc 134 1 349 0             # ../../base/values_unittest.cc:349:0
        .cv_loc 134 1 350 0             # ../../base/values_unittest.cc:350:0
        .cv_loc 134 1 351 0             # ../../base/values_unittest.cc:351:0
        .cv_loc 134 1 351 0             # ../../base/values_unittest.cc:351:0
        .cv_loc 134 1 351 0             # ../../base/values_unittest.cc:351:0
        .cv_loc 134 1 351 0             # ../../base/values_unittest.cc:351:0
        .cv_loc 134 1 352 0             # ../../base/values_unittest.cc:352:0
        .cv_loc 134 1 352 0             # ../../base/values_unittest.cc:352:0
        .cv_loc 134 1 352 0             # ../../base/values_unittest.cc:352:0
        .cv_loc 134 1 352 0             # ../../base/values_unittest.cc:352:0
        .cv_loc 134 1 354 0             # ../../base/values_unittest.cc:354:0
        .cv_loc 134 1 356 0             # ../../base/values_unittest.cc:356:0
        .cv_loc 134 1 357 0             # ../../base/values_unittest.cc:357:0
        .cv_loc 134 1 357 0             # ../../base/values_unittest.cc:357:0
        .cv_loc 134 1 357 0             # ../../base/values_unittest.cc:357:0
        .cv_loc 134 1 357 0             # ../../base/values_unittest.cc:357:0
        .cv_loc 134 1 358 0             # ../../base/values_unittest.cc:358:0
        .cv_loc 134 1 358 0             # ../../base/values_unittest.cc:358:0
        .cv_loc 134 1 358 0             # ../../base/values_unittest.cc:358:0
        .cv_loc 134 1 358 0             # ../../base/values_unittest.cc:358:0
        .cv_loc 134 1 359 0             # ../../base/values_unittest.cc:359:0

But the actual line table in the object file is "jumpy", it goes back to line 349 (Value value(list);) multiple times:

$ cvdump -s -l obj/base/base_unittests/values_unittest.obj
...
(00000C) S_GPROC32_ID: [0000:00000000], Cb: 000007C3, ID:             0x1F5C, base::ValuesTest_MoveList_Test::TestBody
...
*** LINES

  0000:00000000-000007C3, flags = 0000, fileid = 00000000

    347 00000000    348 0000002B    349 00000126    350 0000017E
    351 000001CA    351 0000023D    349 0000024F    351 00000257
    349 000002CC    351 000002D4    352 000002E1    352 00000370
    349 00000382    352 0000038A    349 000003FF    352 00000407
    354 00000414    356 00000440    357 000004F4    357 0000056F
    354 00000581    357 00000589    354 000005F8    357 00000600
    358 0000060D    358 00000693    354 000006A5    358 000006AD
    354 0000071C    358 00000724    359 00000731

It looks like the bug is in the assembler.

Comment 3 by thakis@chromium.org, Jul 31 2017 

We use clang's built-in assembler, right?

Comment 4 by r...@chromium.org, Jul 31 2017 

> We use clang's built-in assembler, right?

Yep. The bug appears to be in llvm/lib/MC.

Comment 5 by r...@chromium.org, Jul 31 2017 

Oops, the assembly I was looking at in c#2 wasn't representative. Now I see assembly like this, which looks a lot more like what Brett was describing:

	.cv_loc	134 1 351 0             # ../../base/values_unittest.cc:351:0
	testb	$1, %al
	jne	.LBB134_3
	jmp	.LBB134_4
.Ltmp913:
.LBB134_3:                              # %if.then
	#DEBUG_VALUE: TestBody:value <- [DW_OP_deref] [%RSP+304]
	jmp	.LBB134_5
.Ltmp914:
.LBB134_4:                              # %if.else
	#DEBUG_VALUE: TestBody:value <- [DW_OP_deref] [%RSP+304]
	.cv_loc	134 1 349 0             # ../../base/values_unittest.cc:349:0
	leaq	536(%rsp), %rcx
.Ltmp915:
	.cv_loc	134 1 351 0             # ../../base/values_unittest.cc:351:0
	callq	"??0Message@testing@@QEAA@XZ"

So, the bug is not in the assembler, it's in codegen.

Comment 6 by r...@chromium.org, Jul 31 2017 

This was actually a corner case of  http://crbug.com/726828 . LLVM knew that the LEA -> RCX didn't have a source location, so we were checking adjacent instructions to find a good one. However, we looked at the #DEBUG_VALUE instruction, which isn't a real instruction, it's just a debug info marker. We shouldn't do that, that was a bug in my fix for  issue 726828 .

After LLVM r309628 the line table should be a lot less jumpy.

Comment 7 by thakis@chromium.org, Jul 31 2017

Blockedon: 750434

Comment 8 by thakis@chromium.org, Aug 9 2017

Owner: r...@chromium.org
Status: Fixed (was: Untriaged)
►

Sign in to add a comment