The `--format` option relied on `click.Context.get_parameter_source` to
determine if the default value for the `--format` parameter was passed.
A bug in click caused this to behave "correctly", click 8.4.0 fixed the
bug. Change the logic by using `None` as the default value so that it
can be checked without using the parameter source.
Since isort 5.11.0 if a import statement ends with a trailing comma it's
excluded for sorting, which makes the black-formatted import lines
unformatted by isort. Disable this behavior in the config so all import
lines are sorted as they should.
Unfortunately, in DWARF, there's no widespread indicator on whether a
function is regular ARM or Thumb. Therefore, in this commit, instead of
blindly creating a new function, we first check if, in the model, we
already have a function at that address.
This means that, if we have a Thumb function in DWARF, but it doesn't
already exist in the model, we will import as regular ARM, which is
wrong.
Specifically, this can happen with `revng model import debug-info`.
We used to use dynamic_symbol_begin which does not use segments (uses
sections) and also tampers with `st_value` for ARM symbols.
This commit makes it reuse the more accurate logic implemented in
ELFImporter.
DW_AT_abstract_origin chains are walked recursively to find a DIE's
name, with no cycle check. Some binaries (e.g. debian-bookworm-mipsel
/usr/sbin/pwck) ship DWARF with a cycle in that chain, so getName ended
up in infinite recursion.
Make the walk iterative.
Smoke test that exercises the linux/x86-64 path: feeds malloc/calloc/
realloc plus a NonExistentFunction through the analysis and asserts that
the typed symbols pick up a prototype while the unknown one does not.
A library's exported functions live in model::Function. Walk the
Functions list and insert one Symbol row per ExportedName so the
database covers both shapes.
Also, batch all symbol inserts.
Add three more unit tests that mirror the previous "with_alloca"
variants but where the store of the extra argument %val into the
pre-existing alloca happens BEFORE the call instead of after it. The
expected number of NEW allocas after the pass is the same as in the
corresponding store-after-call variant: the position of the store
relative to the call does not change the picker's decision.
Add three more unit tests, one for each of the three call patterns
introduced in the previous commit. Each test takes an extra i64
argument %val, has an alloca pre-existing in the entry block, stores
%val into the alloca right after the call, and at least one branch
returns by loading from the alloca.
The pre-existing alloca/store/load pattern does not write to memory
the call could read (the alloca is local and the call's pointer
argument is unrelated) and the call cannot write to the alloca, so
the picker should reach the same conclusion as in the simpler
counterpart tests: the variant of @call_rw_one_use generates no new
alloca, @call_rw_two_uses generates one new alloca alongside the
pre-existing one, and @call_ro_two_uses generates none.
Cover three representative cases for how the picker should treat
function calls in non-legacy mode:
- A call to a read+write function with a single use (an icmp that
feeds a conditional branch). The call has only one use, so the
picker does not pick it and no new alloca is created.
- The same shape, but with two uses of the call (the icmp and a
return). Because the function may read+write memory and the call
has more than one use, the picker picks the call: a new alloca is
created at the top of entry, the result of the call is stored into
it, and every user of the call uses a load from the alloca.
- The same shape as the second case but with a function that only
reads memory (and is also nounwind willreturn so that
Instruction::mayHaveSideEffects returns false). The picker does
not pick the call and no new alloca is created.
Make cannotBePointer recurse through an Add when both of its
operands cannot be pointers. This lets canDisambiguatePointerOperand
identify the only viable base pointer of an outer Add even when the
offset side is itself an Add (or a deeper tree of Adds) whose leaves
are all clearly-non-pointer values, e.g.
%offset = add i64 (%a*%b), (%c*%d)
%result = add i64 %arg, %offset
Two unit tests cover the simple two-mul case and a deeper
four-mul binary-tree variant.
Cover several scenarios where a single base pointer is reachable
only after the discovery walk in ArithmeticToGEPPass propagates
through many layers of Add instructions, each of which has
ambiguous pointer operands and must be disambiguated separately:
- A chain of 8 chained adds with a varied set of clearly-non-pointer
offset opcodes (mul, ashr, shl, and, or, xor, sdiv, srem), with
discovery seeded from the !revng.pointers metadata on the return.
- A chain seeded from a pointer argument (forward propagation).
- A chain feeding the integer argument of a call whose argument is
a pointer (backwards discovery from the call site).
- A chain where the pointer side alternates between operand 0 and
operand 1 of each add.
- A chain whose offset side is hidden behind freeze instructions,
so the cannotBePointer recursion through transparent casts is
exercised.
- A chain where every add reuses the same multiplication as offset,
exercising independent disambiguation per-add over a shared
sub-expression.
- A chain interrupted by ptrtoint/inttoptr round-trips between adds.
The ModuleSlotTracker is owned by a new function-level
ModuleSlotTrackerAnalysis whose Result holds the MST through a
unique_ptr (MST is neither copyable nor movable, but the FAM
cache requires a movable Result). Its run method calls
incorporateFunction on the function.
AvailableExpressionsAnalysis and InstructionToSerializePicker
fetch the MST via FAM.getResult<ModuleSlotTrackerAnalysis>(F) and
forward it down to AvailableExpressionsResult and the monotone
framework, so all dumpToString calls on the logging path share
the same slot-numbering cache.
Mirror the existing pointer overload of dumpToString but thread a
ModuleSlotTracker through to the underlying call, so that callers
can amortise slot-numbering work across many dumps. The new
overload returns "nullptr" when the input pointer is null,
matching the non-MST pointer overload.
Drop the workaround that skipped recursion into a Use whose User
was a CallInst with that Use as an arg operand. The original
motivation no longer holds with the rest of the recent changes,
and the workaround was suppressing valid serialisation
opportunities at call sites.
Remove the workaround that suppressed the redundant
self-assignment that the pass now occasionally introduces in
non-legacy mode.
The remaining self-assignments are not bugs but sub-optimal C
generation, so we just disable the -Wself-assign warning in the
tests.
The sub-optimal code generation comes from passes that emit local
variables (allocas) before conversion to Clift, e.g. STS and
ExitSSA (with more planned). Each of these passes generates local
variables independently and they don't know about each other; the
order between them might also be swapped in the future. The only
clean fix is to unify them into a single transformation pass that
takes the results of multiple analyses (in the LLVM sense) and
emits local variables once. We have plans to do that, and when
it's done the warning can be re-enabled.
Hoist the "use already picked in a previous iteration" early-out
to the very start of the per-use loop in
pickInstructionForMemoryRead, so that the rest of the body never
runs for uses that are going to be replaced anyway. The check
itself is unchanged.
Introduce a new canonicalisation pass that forces a small set of
exponential-dataflow-prone instructions (SelectInst plus the FShl
and FShr intrinsic patterns) to be stored into a dedicated local
variable. Without this, expanding their value into C expressions
during decompilation can blow up into exponential path enumeration
on real binaries.
This is a temporary workaround. A more principled split of
pathological dataflows is planned and will replace this pass.
If an instruction picked for serialisation has zero uses, skip
emitting an alloca/copy pair for it. The Clifter recognises a
zero-use instruction as a statement and turns it into an
ExpressionStatement in place, preserving the ordering as if the
local variable had been emitted, but without the cost of a real
local variable. The legacy mode keeps the previous behaviour.
When a use of `I` is going to be replaced by a copy from
SelectedAssign and `User` does not write memory, there is no need
to recur on uses of `User`: those uses will be reading from
SelectedAssign's location, so the original memory read no longer
flows through them.
This avoids a class of redundant explorations and the spurious
serialisations they used to cause.
The "skip over the Assign operand" workaround is only needed in
legacy mode, where assignments are calls to opaque functions that
the rest of the pass cannot reason about. Gate it on `IsLegacy`
so that the non-legacy code path treats the pointer operand of a
StoreInst as a regular use, which is semantic-preserving and
unblocks downstream simplifications.
Giacomo Vercesi