The DLAStep SimplifyInstanceAtOffset0 now only triggers if the parent
node has the child-at-offset-0 node as its only successor, or when the
child-at-offset-0 doesn't have other predecessors.
Doing this guarantees that it's impossible for another predecessor of
the child-at-offset-0 to start seeing memory accesses that were
initially relative to the parent.
This condition is slightly more restrictive than the previous one, but
it takes into consideration some far reaching consequences.
If SimplifyInstanceAtOffset0 aggressively like we did before, DLA can
end up inferring types in some memory locations, like executable
segments, for which there aren't clues in the binary.
This isn't bad per se, but if DLA does that, the newly recovered type is
identified as non-executable data, causing misdecompilation because
rev.ng doesn't decompile memory regions that it understands as non
executable.
This commit, making SimplifyInstanceAtOffset0 less aggressive, makes it
play better with the rest of the assumptions of the decompilation
pipeline.
It also relaxes a decompilation test that was previously working by
chance and that was effectively beyond the current expressive power of
reasoning for DLA at the moment.
Before this commit the breadth-first traversal was more convoluted and
it used a broken criterion for bailing out, resulting sometimes in
considering a pointer edge as if it was an instance edge.
This in turn caused bugs to the point of the DLA Graph failing
verification.
This commit rewrites and simplifies the main traversal and comparisons
in DeduplicateField to make them robust against these bugs.
End-to-end tests were disabled due to a bug in ModelToHeader that caused
the decompiled C header with the types not to be recompilable in some
cases.
However, the commit that disabled the tests was too aggressive, because
the only part that needed to be disabled was the part of the tests that
actually recompiled the C code.
This commit re-enables the end-to-end tests except for the recompilation
part, that will be re-enabled after the bug in ModelToHeader is fixed.
This is due to the fact that type inlining currently is broken in some
corner cases involving recursive types.
The bugs are caused by the fact that TypeInlineHelper uses its own
custom graph instead of using the DependencyGraph used by ModelToHeader,
causing different decisions about the order of emission of types.
As a result, the generated C types are not valid C, and they fail to
compile because a field of a struct has a type that is defined later
than its use.
After refactoring TypeInlineHelper to use DependencyGraph like
ModelToHeader, this can be reverted.
Before this commit the code was wrong in 2 ways:
- if failed to iterate on children, because it iterated a range that was
effectively [begin, begin) instead of [begin, end)
- the update of the ARootNext iterator in the loop was broken, but it
was impossible to realize that since the loop was never taken
This commit fixes both these intertwined problems.
The FieldAttr::verify checking for non-complete field types was testing
for completeness too eagerly and was removed. Forbidding class type
recursion without indirection is desirable, but needs to be done less
eagerly. A new unit test to validate this via clift::ModuleOp::Verify
needs to be created to replace this one.
This change is a preparation for further decoupling of the
TypeInlineHelper from its custom dependency graph type, to try and reuse
the DependencyGraph used in ModelToHeader.
Pietro Fezzardi