DI-based TypeHierarchy #18
Conversation
unittests/PhasarLLVM/TypeHierarchy/DIBasedTypeHierarchyTest.cpp
Outdated
Show resolved
Hide resolved
|
|
||
| // Initialize the transitive closure matrix with all as false | ||
| llvm::BitVector InitVector(VertexTypes.size(), false); | ||
|
|
There was a problem hiding this comment.
TransitiveClosure.reserve(VertexTypes.size());
unittests/PhasarLLVM/TypeHierarchy/DIBasedTypeHierarchyTest.cpp
Outdated
Show resolved
Hide resolved
unittests/PhasarLLVM/TypeHierarchy/DIBasedTypeHierarchyTest.cpp
Outdated
Show resolved
Hide resolved
| for (size_t I = 0; I < VTableSize; I++) { | ||
| IndexToFunctions.push_back(Init); | ||
| } |
There was a problem hiding this comment.
I don't think, the size of IndexToFunctions is tied to the max virtual index; rather it is the number of types. The sizes of the inner vectors depend on that, but on a per-type basis
| IndexFunctions.push_back(FunctionToAdd); | ||
| // concatenate vectors of functions of this index | ||
| IndexToFunctions[VirtualIndex].insert( | ||
| IndexToFunctions.at(VirtualIndex).begin(), IndexFunctions.begin(), | ||
| IndexFunctions.end()); |
There was a problem hiding this comment.
I would rather go with sth like
if (IndexToFunctions[TypeIndex].size() <=VirtualIndex){
IndexToFunctions[TypeIndex].resize(VirtualIndex + 1);
}
IndexToFunctions[TypeIndex][VirtualIndex] = FunctionToAdd;| for (const auto &Function : VTable.getAllFunctions()) { | ||
| OS << Function->getName() << "\n"; | ||
| OS << Function->getName() << ", "; | ||
| } |
There was a problem hiding this comment.
You may want to take a look at llvm::interleaveComma
| } | ||
| EXPECT_TRUE(DBTH.hasType(DBTH.getType("Base"))); | ||
| EXPECT_TRUE(DBTH.hasType(DBTH.getType("Child"))); | ||
| EXPECT_TRUE(BaseSubTypes.find(DBTH.getType("Child")) != BaseSubTypes.end()); |
There was a problem hiding this comment.
| EXPECT_TRUE(BaseSubTypes.find(DBTH.getType("Child")) != BaseSubTypes.end()); | |
| EXPECT_TRUE(BaseSubTypes.count(DBTH.getType("Child"))); |
| if (VTableForBase->empty()) { | ||
| EXPECT_TRUE(false); | ||
| } else { | ||
| EXPECT_TRUE(VTableForBase->getFunction(0)->getName() == "_ZN4Base3barEv"); | ||
| } |
There was a problem hiding this comment.
| if (VTableForBase->empty()) { | |
| EXPECT_TRUE(false); | |
| } else { | |
| EXPECT_TRUE(VTableForBase->getFunction(0)->getName() == "_ZN4Base3barEv"); | |
| } | |
| ASSERT_FALSE(VTableForBase->empty()); | |
| EXPECT_EQ(VTableForBase->getFunction(0)->getName(), "_ZN4Base3barEv"); |
| llvm::outs() << "Current arg: " << Arg << "\n"; | ||
| llvm::outs().flush(); |
There was a problem hiding this comment.
May use PhASAR's logger here instead
| if (IndexToFunctions[TypeIndex->getSecond()].size() <= VirtualIndex) { | ||
| IndexToFunctions[TypeIndex->getSecond()].resize(VirtualIndex); | ||
| } |
There was a problem hiding this comment.
This seems to be redundant
| llvm::outs() << "TC.size(): " << TransitiveClosure.size() | ||
| << " VT.size(): " << VertexTypes.size(); | ||
| llvm::outs().flush(); |
There was a problem hiding this comment.
We should print such messages to stderr (llvm::errs()) instead.
| EXPECT_TRUE(BaseType); | ||
| if (BaseType) { | ||
| EXPECT_TRUE(DBTH.hasType(BaseType)); | ||
| EXPECT_TRUE(DBTH.hasVFTable(BaseType)); | ||
| } |
There was a problem hiding this comment.
| EXPECT_TRUE(BaseType); | |
| if (BaseType) { | |
| EXPECT_TRUE(DBTH.hasType(BaseType)); | |
| EXPECT_TRUE(DBTH.hasVFTable(BaseType)); | |
| } | |
| ASSERT_NE(nullptr, BaseType); | |
| EXPECT_TRUE(DBTH.hasType(BaseType)); | |
| EXPECT_TRUE(DBTH.hasVFTable(BaseType)); | |
| TypeToVertex.end()); | ||
| size_t BaseTypeVertex = TypeToVertex[DerivedType->getBaseType()]; | ||
|
|
||
| llvm::outs().flush(); |
| size_t BaseTypeVertex = TypeToVertex[DerivedType->getBaseType()]; | ||
|
|
||
| llvm::outs().flush(); | ||
| assert(TransitiveClosure.size() >= BaseTypeVertex); |
|
|
||
| llvm::outs().flush(); | ||
| assert(TransitiveClosure.size() >= BaseTypeVertex); | ||
| assert(TransitiveClosure.size() >= ActualDerivedType); |
There was a problem hiding this comment.
In line 75, I want to access TransitiveClosure at the indices BaseTypeVertex and ActualDerivedType.
Line 75: "TransitiveClosure[BaseTypeVertex][ActualDerivedType] = true;"
Shouldn't I check beforehand if these indices are valid, by checking that the TransitiveClosure.size() is bigger than the indices? That's why I put the asserts in line 73 and line 74 there. Is there something I'm missing?
There was a problem hiding this comment.
you should, but in the equality case you are still accessing out of bounds, as the first element has index 0
There was a problem hiding this comment.
Makes sense, thank you. I interpreted the > as "see above", as in "why?"
| std::vector<std::vector<const llvm::Function *>> IndexToFunctions; | ||
| IndexToFunctions.resize(VertexTypes.size()); |
There was a problem hiding this comment.
there is a constructor that allows you to initialize a std::vector with a particular size
| size_t VTableSize = 0; | ||
| for (const auto &Subprogram : Finder.subprograms()) { | ||
| if (Subprogram->getVirtualIndex() > VTableSize) { | ||
| VTableSize = Subprogram->getVirtualIndex(); | ||
| } | ||
| } | ||
| // if the biggest virtual index is 2 for example, the vector needs to have a | ||
| // size of 3 (Indices: 0, 1, 2) | ||
| VTableSize++; |
| continue; | ||
| } | ||
|
|
||
| const auto *const FunctionToAdd = |
| if (TypeIndex->getSecond() >= IndexToFunctions.size()) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
I'm unsure what I should do to improve this case. Is an assert better here, or should I use the phasar logger with log level "WARNING"?
There was a problem hiding this comment.
I think an assertion should be the better choice. This should never happen and would be a reason to fail hard.
| void DIBasedTypeHierarchy::printAsDot(llvm::raw_ostream &OS) const { | ||
| OS << "digraph TypeHierarchy{\n"; |
There was a problem hiding this comment.
this would benefit from labels, so that you see them in the graph visualization.
There was a problem hiding this comment.
What kind of labels? Do you mean like a legend that tells you how the graph is structured?
There was a problem hiding this comment.
The dot file format allows attributes for nodes and edges, one of which is label. There are also more that allow you to set colors, borders etc. when you plot it to a image or just in xdot.
By doing so you could introduce nodes by their id and then reference by id, for example:
0[label="Base"]
1[label="Child"]
1->0[label="public"]
The label for the edge is just an example. In fact, it might be worth thinking about what would be interesting properties of the edges.
This dump would be helpful for debugging, as you see the mapping between ids and names.
There was a problem hiding this comment.
I will implement this, thank you
| TEST(DBTHTest, BasicTHReconstruction_2) { | ||
| LLVMProjectIRDB IRDB({unittest::PathToLLTestFiles + | ||
| "type_hierarchies/type_hierarchy_17_cpp_dbg.ll"}); |
There was a problem hiding this comment.
the indices are inconsistent, that's very surprising and confusing. Is there a reason you omit the other cases? What about case 16 (ll index) ?
| // Since Child2 hasn't been created, it shouldn't exist and also not be found | ||
| // via DBTH.getType("Child2") | ||
| const auto &Child2Type = DBTH.getType("Child2"); | ||
| EXPECT_FALSE(Child2Type); |
There was a problem hiding this comment.
is that intentional? here you are relying on compiler behavior.
| size_t CurrentRowIndex = 0; | ||
| for (const auto &Row : TransitiveClosure) { | ||
| for (const auto &Cell : Row.set_bits()) { | ||
| if (Row[Cell]) { |
There was a problem hiding this comment.
actually this is always true. set_bits() lets you iterate over all the positions where 1 is set.
…LLVMTypeHierarchy
|
Closed in favor of secure-software-engineering#702 |
See secure-software-engineering#623