From f30780a9de8032c877437126aef5f518a08c94a8 Mon Sep 17 00:00:00 2001 From: Andy McFadden Date: Fri, 4 Sep 2020 14:12:31 -0700 Subject: [PATCH] Fix Message update when broken symbolic ref is fixed Renaming a user label doesn't cause a re-analysis, just a display update, because nothing structural is changing. However, that's not quite true when you have a reference to a non-existent label (e.g. "LDA hoser"), and you rename a label to match (e.g. change "blah" to "hoser"). The most obvious consequence was that the Message list, which enumerates the broken symbolic references, was not being updated. We now identify broken references during the refactoring rename, and change the reanalysis mode accordingly. There is a deeper problem, where undoing the label rename does the wrong thing with the previously-broken symbolic references (in the earlier example, it "undoes" them to "blah" rather than back to "hoser"). I added some notes about that, but it's harder to fix. Also, clean up some code that was still treating ReanalysisScope as if it were bit flags. --- SourceGen/DisasmProject.cs | 31 +++++++++++++++++++++++++------ SourceGen/UndoableChange.cs | 24 +++++++++++++++++++++++- 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/SourceGen/DisasmProject.cs b/SourceGen/DisasmProject.cs index 33bff66..e01182d 100644 --- a/SourceGen/DisasmProject.cs +++ b/SourceGen/DisasmProject.cs @@ -1755,7 +1755,7 @@ namespace SourceGen { mAnattribs[offset].Symbol = newSym; SymbolTable.Remove(oldSym); SymbolTable.Add(newSym); - RefactorLabel(offset, oldSym.Label); + RefactorLabel(offset, oldSym.Label, out bool unused); } } } @@ -2244,7 +2244,16 @@ namespace SourceGen { if (oldValue != null) { // Update everything in Anattribs and OperandFormats that // referenced the old symbol. - RefactorLabel(offset, ((Symbol)oldValue).Label); + RefactorLabel(offset, ((Symbol)oldValue).Label, + out bool foundExisting); + + // If we fixed one or more broken references, we need to do + // a deeper re-analysis. + if (foundExisting) { + Debug.WriteLine("Found existing broken ref to '" + + ((Symbol)newValue).Label + "'"); + needReanalysis = UndoableChange.ReanalysisScope.DataOnly; + } } // Compute the affected offsets. We have one special case to @@ -2259,7 +2268,7 @@ namespace SourceGen { IsInProjectOrPlatformList((Symbol)newValue)) { Debug.WriteLine("Label change masked/unmasked " + "project/platform symbol"); - needReanalysis |= UndoableChange.ReanalysisScope.DataOnly; + needReanalysis = UndoableChange.ReanalysisScope.DataOnly; } else { affectedOffsets.Add(offset); @@ -2291,7 +2300,7 @@ namespace SourceGen { if (mScriptManager.IsLabelSignificant((Symbol)oldValue, (Symbol)newValue)) { Debug.WriteLine("Plugin claims symbol is significant"); - needReanalysis |= UndoableChange.ReanalysisScope.CodeAndData; + needReanalysis = UndoableChange.ReanalysisScope.CodeAndData; } } break; @@ -2456,7 +2465,9 @@ namespace SourceGen { default: break; } - needReanalysis |= uc.ReanalysisRequired; + if (needReanalysis < uc.ReanalysisRequired) { + needReanalysis = uc.ReanalysisRequired; + } } return needReanalysis; @@ -2477,7 +2488,10 @@ namespace SourceGen { /// /// Offset with the just-renamed label. /// Previous value. - private void RefactorLabel(int labelOffset, string oldLabel) { + /// Set to true if we found an existing (broken) reference + /// to the new label. + private void RefactorLabel(int labelOffset, string oldLabel, out bool foundExisting) { + foundExisting = false; if (!mXrefs.TryGetValue(labelOffset, out XrefSet xrefs)) { // This can happen if you add a label in a file section that nothing references, // and then rename it. @@ -2502,6 +2516,11 @@ namespace SourceGen { // override that and display as e.g. hex. continue; } + if (Label.LABEL_COMPARER.Equals(newLabel, dfd.SymbolRef.Label)) { + // We found an existing, previously-broken reference to the new label. + // Let the caller know. + foundExisting = true; + } if (!Label.LABEL_COMPARER.Equals(oldLabel, dfd.SymbolRef.Label)) { // This can happen if the xref is based on the operand offset, // but the user picked a different symbol. The xref generator diff --git a/SourceGen/UndoableChange.cs b/SourceGen/UndoableChange.cs index e349bf0..d5f16f4 100644 --- a/SourceGen/UndoableChange.cs +++ b/SourceGen/UndoableChange.cs @@ -58,7 +58,8 @@ anything in the DisasmProject. *** When can we get away with only updating part of the display list (re-analysis=none)? - Changing a user label. All lines that reference the label need to be updated in the display, but nothing in the analysis changes. (This assumes we prevent you from renaming -a label to be the same as an existing label, e.g. auto-generated labels.) +a label to be the same as an existing label, e.g. auto-generated labels.) This doesn't +work quite right when broken symbolic references suddenly become un-broken, however. - Adding/removing/changing cosmetic items, like comments and notes. NOTE: all re-analysis requirements are symmetric for undo/redo. Undoing a change requires @@ -116,6 +117,9 @@ namespace SourceGen { /// /// Enum indicating what needs to be reanalyzed after a change. /// + /// + /// These must be in ascending order of thoroughness. + /// public enum ReanalysisScope { None = 0, DisplayOnly, @@ -280,6 +284,24 @@ namespace SourceGen { /// /// Creates an UndoableChange for a label update. /// + /// + /// TODO(maybe): this is a little broken when it comes to bad symbolic references. + /// + /// If you set an operand's symbol to a non-existent label "foo1", and then rename an + /// existing user from "bar1" to "foo1", the bad references become valid. However, we + /// set the reanalysis mode to "None" here, which means the Message list and + /// cross-references aren't updated. We can catch this easily during the update, in + /// the refactoring rename, and just change the reanalysis mode. + /// + /// However... in the above scenario, if you undo the change, we perform a refactoring + /// undo. The reference to "foo1" becomes a reference to "bar1" instead of a broken + /// reference to "foo1". This happens because we don't keep track of which references + /// were valid and which were previously broken. + /// + /// The fundamental problem is the refactoring rename done way deep down. The proper + /// fix is probably to perform the refactoring at a higher level, and generate an + /// explicit change object for every symbol that is updated. + /// /// Affected offset. /// Current label. May be null. /// New label. May be null.