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.