Page MenuHomePhabricator

[DWARF] Assign the correct scope to constant variables
ClosedPublic

Authored by davide on Apr 7 2020, 5:48 PM.

Details

Summary

SymbolFileDWARF::ParseVariableDIE consider all constant variables as "static".
This is incorrect, and causes frame var to fail when printing them.

e.g.

volatile int a;	
   main() {
     	  {
     	    int b = 3;
   	    a;
   	  }
   	}

const_value variables are more common at -O1/-O2/.., so this wasn't noticed by many.

Fixes rdar://problem/61402307

I also need to craft a testcase. Ideas on how to do this are appreciated

Diff Detail

Event Timeline

davide created this revision.Apr 7 2020, 5:48 PM
davide added a reviewer: friss.Apr 7 2020, 5:50 PM

I also need to craft a testcase. Ideas on how to do this are appreciated

You could check in the assembly generated by clang for this test case and then run llvm-mc && lldb-test symbols over it. Currently I get something like Variable{0x7fffffff0000007f}, name = "b", type = {7fffffff00000044} 0x0000564FADF7F9F0 (int), scope = ??? (2) when running that. After this patch that should presumably say scope = local. That won't actually check that the "frame variable" command succeeds, but maybe that's enough for this patch?

Using llvm ir instead of assembly might work too .. the relevant ir (call void @llvm.dbg.value(metadata i32 3, metadata !16, metadata !DIExpression()), !dbg !18) generates DW_AT_const_value even without any further optimizations, and it looks like it could be reasonably expected to keep doing that in the future. Adding @aprantl for IR insights.

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
1028–1032

This comment does not make sense in the new setting.

davide updated this revision to Diff 256063.Apr 8 2020, 10:23 AM

Added test, updated comment.

jingham accepted this revision.Apr 8 2020, 11:03 AM

The code looks good to me. I'll defer to Adrian as to whether the test is robust, though it also seems reasonable to deal with a failure caused by llvm changes when it happens.

This revision is now accepted and ready to land.Apr 8 2020, 11:03 AM
davide closed this revision.Apr 8 2020, 11:08 AM

commit d51b38f1b3a34c2a8e1869af6434ebd743ce7a5e (HEAD -> master, origin/master, origin/HEAD)
Author: Davide Italiano <ditaliano@apple.com>
Date: Wed Apr 8 11:06:00 2020 -0700

[DWARF] Not all the constant variables are "static".

Fixes rdar://problem/61402307

Differential Revision: https://reviews.llvm.org/D77698
aprantl added inline comments.Apr 9 2020, 3:05 PM
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
1028

[Sorry for showing up late.]

This seems unintuitive. How is

void f() {
static int g_i = 0;
}

represented in DWARF?

1029

To save everyone the trouble:

0x0000002a:   DW_TAG_subprogram
                DW_AT_low_pc	(0x0000000000000000)
                DW_AT_high_pc	(0x0000000000000006)
                DW_AT_frame_base	(DW_OP_reg6 RBP)
                DW_AT_name	("f")
                DW_AT_decl_file	("/tmp/t.c")
                DW_AT_decl_line	(1)
                DW_AT_external	(true)

0x0000003f:     DW_TAG_variable
                  DW_AT_name	("g_i")
                  DW_AT_type	(0x00000055 "int")
                  DW_AT_decl_file	("/tmp/t.c")
                  DW_AT_decl_line	(2)
                  DW_AT_location	(DW_OP_addr 0x280)

0x00000054:     NULL
davide marked an inline comment as done.Apr 9 2020, 3:25 PM
davide added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
1029

hmm, so, what's the DWARF'y way to find out whether a variable is static?
This code is just moved from another place which did this.

aprantl added inline comments.Apr 9 2020, 3:42 PM
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
1029

Looks like the function's name is just confusing. We should add a doxgygen comment for it,