Page MenuHomePhabricator

Testcase and fix for bug 24074
ClosedPublic

Authored by ravitheja on Aug 27 2015, 4:39 AM.

Details

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.

Diff Detail

Event Timeline

ravitheja updated this revision to Diff 33312.Aug 27 2015, 4:39 AM
ravitheja retitled this revision from to Testcase and fix for bug 24074 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....
ravitheja updated this object.
ravitheja retitled this revision from Testcase and fix for bug 24074 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... to Testcase and fix for bug 24074.Aug 27 2015, 4:53 AM
ravitheja updated this object.
clayborg requested changes to this revision.Sep 8 2015, 9:44 AM
clayborg edited edge metadata.

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
This revision now requires changes to proceed.Sep 8 2015, 9:44 AM
ravitheja added a comment.EditedSep 9 2015, 5:06 AM

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 updated this revision to Diff 35228.Sep 21 2015, 4:08 AM
ravitheja edited edge metadata.

Renamed TypeList to TypeMap
and created SortTypeList function

clayborg requested changes to this revision.Sep 21 2015, 11:35 AM
clayborg edited edge metadata.

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

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
44–47

Go ahead and remove this function.

125–126

Remove this.

This revision now requires changes to proceed.Sep 21 2015, 11:35 AM
ravitheja updated this revision to Diff 35347.Sep 22 2015, 12:39 AM
ravitheja edited edge metadata.
ravitheja marked 2 inline comments as done.

Moved SortType to FindTypes

ravitheja marked an inline comment as done.Sep 22 2015, 12:46 AM
clayborg accepted this revision.Sep 22 2015, 9:23 AM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Sep 22 2015, 9:23 AM
ravitheja closed this revision.Sep 23 2015, 12:20 AM
emaste added a subscriber: emaste.Sep 23 2015, 10:15 AM

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

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.