Testcase and fix for bug 24074

[ravitheja]

Summary

In bug 24074, the type information is not shown
correctly. This commit includes the following -
-> Changes for displaying correct type based on

current lexical scope for the command "image
lookup -t"

-> The corresponding testcase.

[clayborg]

See inlined comments.

source/Commands/CommandObjectTarget.cpp

1911–1943

I would rather see this promotion of the correct types happen inside Module::FindTypes() so that we don't need to do this everywhere.

Any types that are returned in the type list should be ordered such that the first item in the list of the most important and correct choice for the SymbolContext that was provided.

When checking the symbol context block, we also need to check the parent blocks and stop when we run into a block that has inlined function info. So the solution in Module::FindTypes() should probably pass the resulting unsorted TypeList with all types to a new SymbolContext function:

TypeList
SymbolContext::SortTypeList (const TypeList& type_list);

The block in the symbol context might be the deepest block in a function:

int main(int argc, char **argv)
{
    if (argc)
    {
        if (argv[argc])
        { // <<< SymbolContextBlock might be this one
        }
    }
}

So we will need to check for the best block as a type could be defined just inside main, or inside the "if (argc)" block or inside the "if (argv[argc])" block.

There is a function on a block where you can check if a block contains another block as a child:

bool
Block::Contains (const Block *block) const;

Because you might end up with code where a type is defined in a block in a function, but it isn't contained in any parent blocks of the current block...

So the type list ordering should be:

• if there is a block in the SymbolContext: first all of the types that are in the current block or any parent block of the current block and stop when you hit the block that is the function block (Block *SymbolContext::GetFunctionBlock()). Deepest block matches should come first, followed by blocks above in the exact order.
• if there is a function block in the SymbolContext, any other types defined in the same function block, just not in the correct block hierarchy from above, no ordering required for all items at this level
• if there is a compile unit in the SymbolContext, any types defined in the current compile unit (no order preference between these)
• if there is a module in the SymbolContext, any type matches from the current module (no order preference between these)
• all other matches from other modules, no specific order

[ravitheja]

The TypeList is a multimap that uses the USERID as the key, so the order in which the types would be stored is always fixed and sorting may not be useful. I can make modifications for Inlined functions ?.

[ravitheja]

Renamed TypeList to TypeMap
and created SortTypeList function

[clayborg]

Very close, we just need the FindTypes_Impl to be left as they were (but switch over to using TypeMap, and then have the Module::FindTypes() and ModuleList::FindTypes() do the actual sorting before the results are returned.

source/Core/Module.cpp

949–956 ↗

(On Diff #35228)

indents should be four spaces on all these.

You also have to watch out for the "append" argument and do the right thing if it is "true" and append any matches from "typesmap".

Also, the _Impl versions of these functions should be left as is, no changes except that it should use TypeMap like it was before, but then the places that call this _Impl should then do the sorting. The issue is Module has FindTypes(...) and ModuleList also have FindTypes(...). Each of these FindTypes() functions calls the _Impl versions, and they should be the ones that do the final sorting after all results have been gathered.

source/Symbol/TypeList.cpp

48–66 ↗

(On Diff #35228)

Go ahead and remove this function.

145–158 ↗

(On Diff #35228)

Remove this.

[ravitheja]

Moved SortType to FindTypes

[clayborg]

Looks good.

[emaste]

There seems to be additional changes in the committed version (rL248366) not reflected in this review?
rL248366 also introduced segfaults in about 10 testcases on FreeBSD

[granata.enrico]

I reverted this in revision 248421 because it was causing breakages in the test suite.

For future reference, please remember to run the test suite before committing changes.

source/Commands/CommandObjectTarget.cpp

 {
     if (!sym_ctx.module_sp)
         return 0;
 
     TypeList type_list;
     const uint32_t max_num_matches = UINT32_MAX;
     size_t num_matches = 1;
     bool name_is_fully_qualified = false;
+    uint32_t best_match_idx = 0;
 
     ConstString name(name_cstr);
     num_matches = sym_ctx.module_sp->FindTypes(sym_ctx, name, name_is_fully_qualified, max_num_matches, type_list);
 
     if (num_matches)
     {
         strm.Indent ();
         strm.PutCString("Best match found in ");
         DumpFullpath (strm, &sym_ctx.module_sp->GetFileSpec(), 0);
         strm.PutCString(":\n");
-
-        TypeSP type_sp (type_list.GetTypeAtIndex(0));
+        Block * curr_block = sym_ctx.block;
+        bool found = false;
+        while (curr_block != nullptr)
+        {
+            for (uint32_t i = 0; i < num_matches; i++)
+            {
+                TypeSP type_sp_temp (type_list.GetTypeAtIndex(i));
+                if (curr_block == type_sp_temp->GetSymbolContextScope()->CalculateSymbolContextBlock())
+                {
+                    best_match_idx = i;
+                    found = true;
+                    break;
+                }
+            }
+            if (found)
+                break;
+            else
+            {
+                curr_block = curr_block->GetParent();
+            }
+        }
+        TypeSP type_sp (type_list.GetTypeAtIndex(best_match_idx));
         if (type_sp)
         {
             // Resolve the clang type so that any forward references
             // to types that haven't yet been parsed will get parsed.
             type_sp->GetFullCompilerType ();
             type_sp->GetDescription (&strm, eDescriptionLevelFull, true);
             // Print all typedef chains
             TypeSP typedef_type_sp (type_sp);

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

SymbolFileDWARF::GetTypeForDIE (const DWARFDIE &die)
     if (die)
     {
         Type *type_ptr = m_die_to_type.lookup (die.GetDIE());
         if (type_ptr == NULL)
         {
             CompileUnit* lldb_cu = GetCompUnitForDWARFCompUnit(die.GetCU());
             assert (lldb_cu);
             SymbolContext sc(lldb_cu);
+            const DWARFDebugInfoEntry* parent_die = die.GetParent().GetDIE();
+            while (parent_die != nullptr)
+                {
+                    if (parent_die->Tag() == DW_TAG_subprogram)
+                        break;
+                    parent_die = parent_die->GetParent();
+                }
+            SymbolContext sc_backup = sc;
+            if (parent_die != nullptr && !GetFunction(DWARFDIE(die.GetCU(),parent_die), sc))
+                sc = sc_backup;
+
             type_sp = ParseType(sc, die, NULL);
         }
         else if (type_ptr != DIE_IS_BEING_PARSED)
         {
             // Grab the existing type from the master types lists
             type_sp = type_ptr->shared_from_this();
         }
 

test/lang/c/typedef/Makefile

+LEVEL = ../../../make
+
+C_SOURCES := main.c
+
+include $(LEVEL)/Makefile.rules

test/lang/c/typedef/Testtypedef.py

+"""Look up type information for typedefs of same name at different lexical scope and check for correct display."""
+
+import os, time
+import unittest2
+import lldb
+from lldbtest import *
+import lldbutil
+
+class TypedefTestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    @skipUnlessDarwin
+    @dsym_test
+    @expectedFailureClang("llvm.org/pr19238")
+    def test_with_dsym(self):
+        """Test 'image lookup -t a' and check for correct display at different scopes."""
+        self.buildDsym()
+        self.image_lookup_for_multiple_typedefs()
+
+    @dwarf_test
+    @expectedFailureClang("llvm.org/pr19238")
+    def test_with_dwarf(self):
+        """Test 'image lookup -t a' and check for correct display at different scopes."""
+        self.buildDwarf()
+        self.image_lookup_for_multiple_typedefs()
+
+    def image_lookup_for_multiple_typedefs(self):
+        """Test 'image lookup -t a' at different scopes and check for correct display."""
+        exe = os.path.join(os.getcwd(), "a.out")
+        self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+        typearray = ("float", "float", "char", "float", "int", "double", "float", "float")
+        arraylen = len(typearray)+1
+        for i in range(1,arraylen):
+            loc_line = line_number('main.c', '// Set break point ' + str(i) + '.')
+            lldbutil.run_break_set_by_file_and_line (self, "main.c",loc_line, num_expected_locations=1, loc_exact=True)
+
+        self.runCmd("run", RUN_SUCCEEDED)
+
+        for t in typearray:
+            self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+            substrs = ['stopped', 'stop reason = breakpoint'])
+            self.expect("image lookup -t a", DATA_TYPES_DISPLAYED_CORRECTLY,
+            substrs = ['name = "' + t + '"'])
+            self.runCmd("continue")
+
+if __name__ == '__main__':
+    import atexit
+    lldb.SBDebugger.Initialize()
+    atexit.register(lambda: lldb.SBDebugger.Terminate())
+    unittest2.main()

test/lang/c/typedef/main.c

+//===-- main.c --------------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+int main (int argc, char const *argv[])
+{
+    typedef float a;
+    int i = 0; // Set break point 1.
+    i++;
+    a floatvariable = 2.7; // Set break point 2.
+    {
+        typedef char a;
+        i++;
+        a charvariable = 'a'; // Set break point 3.
+    }
+    {
+        int c = 0;
+        c++; // Set break point 4.
+        for(i = 0 ; i < 1 ; i++)
+        {
+            typedef int a;
+            a b;
+            b = 7; // Set break point 5.
+        }
+        for(i = 0 ; i < 1 ; i++)
+        {
+            typedef double a;
+            a b;
+            b = 3.14; // Set break point 6.
+        }
+        c = 1; // Set break point 7.
+    }
+    floatvariable = 2.5;
+    floatvariable = 2.8; // Set break point 8.
+    return 0;
+}