This is an archive of the discontinued LLVM Phabricator instance.

[llvm-dwarfdump][test] Add a --statistics test for a DW_AT_artificial variable
ClosedPublic

Authored by MaskRay on Aug 16 2020, 5:27 PM.

Diff Detail

Event Timeline

MaskRay created this revision.Aug 16 2020, 5:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2020, 5:27 PM
MaskRay requested review of this revision.Aug 16 2020, 5:27 PM
dblaikie added inline comments.
llvm/test/tools/llvm-dwarfdump/X86/statistics.ll
26–29

Does this function need a return value or would a void-returning member function suffice?

MaskRay added inline comments.Aug 17 2020, 11:35 AM
llvm/test/tools/llvm-dwarfdump/X86/statistics.ll
26–29

void T::address() { (void)this; } works as well.

void T::address() { this; } (-Wunused-variable) does not generate this

It is also interesting that the !dbg metadata on ret void, !dbg !17 is important. Dropping the metadata will cause the absence of this as a DW_TAG_formal_parameter (I think it has something to do with DW_AT_location (DW_OP_fbreg -8))

dblaikie added inline comments.Aug 17 2020, 11:42 AM
llvm/test/tools/llvm-dwarfdump/X86/statistics.ll
26–29

Is the IR being produced with optimizations enabled? Because at least for a simple test it looks like a location is generated even for a function that never names/mentions "this" (& if adding "(void)this;" changes the DWARF generation I'd be surprised/concerned, really - I don't think it should change the DWARF or the code generated)

$ cat test.cpp
struct t1 { void f1(); }; void t1::f1() { }
$ clang++-tot test.cpp -g -c && llvm-dwarfdump-tot test.o
test.o: file format elf64-x86-64

.debug_info contents:
0x00000000: Compile Unit: length = 0x00000070, format = DWARF32, version = 0x0004, abbr_offset = 0x0000, addr_size = 0x08 (next unit at 0x00000074)

0x0000000b: DW_TAG_compile_unit
              DW_AT_producer    ("clang version 12.0.0 (git@github.com:llvm/llvm-project.git 545abf55a70686cd1335a95075bc2c6eba8b135d)")
              DW_AT_language    (DW_LANG_C_plus_plus_14)
              DW_AT_name        ("test.cpp")
              DW_AT_stmt_list   (0x00000000)
              DW_AT_comp_dir    ("/usr/local/google/home/blaikie/dev/scratch")
              DW_AT_low_pc      (0x0000000000000000)
              DW_AT_high_pc     (0x000000000000000a)

0x0000002a:   DW_TAG_structure_type
                DW_AT_calling_convention        (DW_CC_pass_by_value)
                DW_AT_name      ("t1")
                DW_AT_byte_size (0x01)
                DW_AT_decl_file ("/usr/local/google/home/blaikie/dev/scratch/test.cpp")
                DW_AT_decl_line (1)

0x00000033:     DW_TAG_subprogram
                  DW_AT_linkage_name    ("_ZN2t12f1Ev")
                  DW_AT_name    ("f1")
                  DW_AT_decl_file       ("/usr/local/google/home/blaikie/dev/scratch/test.cpp")
                  DW_AT_decl_line       (1)
                  DW_AT_declaration     (true)
                  DW_AT_external        (true)

0x0000003e:       DW_TAG_formal_parameter
                    DW_AT_type  (0x00000045 "t1*")
                    DW_AT_artificial    (true)

0x00000043:       NULL

0x00000044:     NULL

0x00000045:   DW_TAG_pointer_type
                DW_AT_type      (0x0000002a "t1")

0x0000004a:   DW_TAG_subprogram
                DW_AT_low_pc    (0x0000000000000000)
                DW_AT_high_pc   (0x000000000000000a)
                DW_AT_frame_base        (DW_OP_reg6 RBP)
                DW_AT_object_pointer    (0x00000061)
                DW_AT_specification     (0x00000033 "_ZN2t12f1Ev")

0x00000061:     DW_TAG_formal_parameter
                  DW_AT_location        (DW_OP_fbreg -8)
                  DW_AT_name    ("this")
                  DW_AT_type    (0x0000006e "t1*")
                  DW_AT_artificial      (true)

0x0000006d:     NULL
MaskRay added inline comments.Aug 17 2020, 12:01 PM
llvm/test/tools/llvm-dwarfdump/X86/statistics.ll
26–29

Reperformed my experiment. You are right: this does not need to appear in the C++ file. I'll drop this.

My other comment about !dbg metadata on ret void, !dbg !17 is correct.

Sorry, looking at the test now - is there any particular reason to test this case in particular? Looks like there's no particular statistics for artificial variables & I guess the statistics code doesn't look at this property one way or another?

MaskRay updated this revision to Diff 286103.Aug 17 2020, 12:07 PM

Simply the test to void T::empty() {}

Sorry, looking at the test now - is there any particular reason to test this case in particular? Looks like there's no particular statistics for artificial variables & I guess the statistics code doesn't look at this property one way or another?

Statistics.cpp:343 has a different DW_AT_artificial use. If we change Statistics.cpp:564 to use NumArtificial in IsFunction case, a DW_AT_artificial variable will be significant.

Sorry, looking at the test now - is there any particular reason to test this case in particular? Looks like there's no particular statistics for artificial variables & I guess the statistics code doesn't look at this property one way or another?

Statistics.cpp:343 has a different DW_AT_artificial use. If we change Statistics.cpp:564 to use NumArtificial in IsFunction case, a DW_AT_artificial variable will be significant.

Not sure I follow, sorry. Are you planning to add a new statistic?

Sorry, looking at the test now - is there any particular reason to test this case in particular? Looks like there's no particular statistics for artificial variables & I guess the statistics code doesn't look at this property one way or another?

Statistics.cpp:343 has a different DW_AT_artificial use. If we change Statistics.cpp:564 to use NumArtificial in IsFunction case, a DW_AT_artificial variable will be significant.

Not sure I follow, sorry. Are you planning to add a new statistic?

No. The new test checks that this (even if not written) is counted as a source variable.

dblaikie accepted this revision.Aug 17 2020, 12:33 PM

Ah, I see - variables are classified as artificial, constant, or local. Even though there's no artificial variable statistic, as such.

I think it might be better if the code tracked the total vars directly - rather than classifying them into 3 groups then merging those again later - if some code decides to have a 4th classification, this merging could get missed & become buggy, compared to maintaining it all along as a separate stat.

Also it looks like artificial parameters aren't getting tracking for all the other stuff - "NumParams", "NumParamTypes", "NumParamSourceLocations", "NumParamLocations"... which seems questionable. Wonder if there was any particular thinking around/justification for that in the original commits of this functionality.

(that said, I haven't had much to do with the Statistics code - so perhaps other folks have other ideas)

This revision is now accepted and ready to land.Aug 17 2020, 12:33 PM