Fix slow basic block export and resume handling#359
Open
Raincarnator wants to merge 1 commit into
Open
Conversation
Owner
|
AI generated explanations are terrible to read and actually magnitudes worse to understand than human made explanations. Please don't again. Thanks. |
Author
|
I'm really sorry. English isn't my first language, and in such long and specialized explanations, using a translator directly would lose the context, so I had to use Codex to generate the explanation and then translate it back into Chinese to check the explanation. Sorry for adding extra work for you, feel free to ask me anything if needed! |
Owner
|
I understand you, don't worry. I'm not a native English speaker either and I understand how hard can it be to explain deep technical problems in something that is not your main language. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes #305 and several related export-time and crash-resume issues found while investigating very large Diaphora exports.
The work was completed jointly by Raincarnator and Codex.
Changes
1. Remove the per-basic-block
get_bb_id()queryinsert_basic_blocks_to_database()used to resolve every basic block id by queryingbasic_blockswith its address before inserting thebb_instructionsrelation.The historical behavior appears to have made sense in older schema versions, where a basic block address uniquely identified a row in
basic_blocks. In Diaphora 2.1.0, for example, the table was effectively modeled like this:With that schema,
get_bb_id(address)was a reasonable safeguard: if a basic block row for that address had already been inserted, the exporter could reuse it; if not, it inserted a new row. Becauseaddresswas unique, the lookup had unambiguous semantics, and because the column was unique/indexed, the lookup was cheap.That assumption no longer holds in the current schema.
basic_blocksnow also storesasm_type:The same address can now legitimately appear more than once, for example once as native basic block data and once as microcode basic block data. The address uniqueness assumption is therefore gone, and the old address lookup is no longer a reliable global identity lookup. More importantly, the current
get_bb_id()query only filters byaddress:It does not filter by
asm_type, and it does not restrict the row to the current function. In the current schema, such a query may return a row from another representation, or another context that happens to share the same address. So the old logic is not only expensive on large exports after the address uniqueness/index assumption disappeared; it is also less precise than using the row that was just inserted.For the native basic-block export path fixed here, re-querying by address is not necessary. The exporter is already executing this sequence locally:
basic_blocks;bb_instructions,bb_relations, andfunction_bblocksrows that should point to that newly inserted native basic block.In this sequence,
cursor.lastrowidis exactly the id that the exporter needs. It identifies the row inserted by the immediately precedinginsert into basic_blocksstatement on the same cursor. Re-querying by address is therefore not adding correctness for this relation; it tries to rediscover the same local row through a slower and now ambiguous key.This does mean that
basic_blocksmay contain multiple rows with the same address. In the current schema that is already allowed and expected, becauseasm_typemakes address alone insufficient as a row identity. The exported graph queries already go throughfunction_bblocksandbasic_block_id, and also filter byasm_type, so each function should use the specific basic block rows linked to it rather than a globally shared row found by address alone.Experiment result:
basic_blocksby address to directly using the inserted row id;Expected effect:
2. Fix typed
DIAPHORA_*option parsingget_value_for()previously checkedisinstance(value, type(default)), but environment variables are strings. This meant bool/int-like settings were not reliably converted.This was especially problematic for boolean values. For example, an environment value such as
"0"was still a non-empty string, so code paths that treated it as a boolean could behave as enabled instead of disabled.This PR:
1/true/yes/onbecome true; values such as0/false/no/offdo not);Expected effect:
DIAPHORA_EXPORT_MICROCODE=0can correctly disable microcode.3. Fix automatic export microcode option name
Automatic export read
"self.export_microcode"instead of"export_microcode", soDIAPHORA_EXPORT_MICROCODEwas ignored.Expected effect:
Experiment result:
4. Avoid duplicate microcode generation
Microcode was generated once inside
decompile_and_get()and then again inextract_all_features().This PR removes the earlier generation and keeps the later one, so microcode is produced once and then immediately consumed by
extract_microcode().Expected effect:
Experiment result:
5. Make crash resume safer
do_export()now returns success explicitly, andexport()only finalizes the DB after a successful export.Expected effect:
6. Preserve crashed export DBs in automatic mode
Automatic export now keeps an existing output DB when the matching
output.sqlite-crashmarker exists.Expected effect:
7. Improve progress dialog refresh cadence
The progress dialog still updates by percentage, but now also refreshes about once every 60 seconds.
Expected effect:
Verification
python -m py_compile diaphora.py diaphora_ida.pygit diff --check -- diaphora.py diaphora_ida.py