inconsistent behavior when renaming class structs

[goatshriek]

Describe the bug
When renaming a structure that is tied to a class (that is, has the same name), the immediate results are different based on whether it is done via the Data Type Manager tree vs. the Structure Editor. Note that the long-term result (that is, once you have saved, exited, and re-loaded) is the same either way.

To Reproduce
Steps to reproduce the behavior:

1. Open CodeBrowser with a program written in C++ and navigate to any call that is in a class namespace and uses thiscall.
2. Right-click the this parameter and choose the "Edit Data Type" menu option.
3. Rename the structure to something different (and unique) and click the Save icon.
4. In the Decompiler window, a low-level decompiler error is displayed, as the type resolution uses the class name for the struct name, and it no longer exists: Low-level Error: Unable to resolve type: <original struct name>
5. Undo this change to recover the decompilation.
6. Find the same structure in the Data Type Manager tree, right click, and select the "Rename" menu option.
7. Rename the structure and press Enter.
8. No decompiler error is encountered - the name of the type changes in the decompilation, no longer matching the class name. However, this is a false state...
9. Save and exit CodeBrowser.
10. Relaunch with the same program and navigate to the function again. The same low-level decompilation error as before is now displayed.
11. Even once you realize that the class name needs to be changed to match, doing so from the Symbol Tree does not clear the error immediately once it is encountered - you must save/close/reopen to successfully rename the class/struct pair and restore decompilation.

Expected behavior
At a minimum, the rename should have the same effect regardless of the menu used. The lack of an error via the Data Type Manager might give a false idea that the struct name doesn't need to match the class name if done this way, and could lead to confusion when the program is re-opened later to suddenly be broken. Renaming the class to match should also immediately clear the error, rather than require a re-opening.

Perhaps an even better solution would be that any attempt to rename a struct tied to a class currently prompts the user to also rename the class as well, regardless of which menu it is done from. It's a very likely sequence of actions to want to take, particularly when using auto-generated class structures, and needing to know the quirk of doing both from the Symbol Tree/Data Type Manager and not the Structure Editor isn't very intuitive.

Environment (please complete the following information):

• OS: Windows 10.0.19043
• Java Version: 16.0.1
• Ghidra Version: 10.0.4 (also tested with fresh clone of master)
• Ghidra Origin: Pulled from Github releases/cloned from master

Additional context
I'm happy to implement the prompt for the class rename if that is deemed beneficial. But I don't want to put the time into that until I know it's something that would be considered for acceptance. Or perhaps there's a better solution I don't know about!

[ghidra1]

This is a known issue. There is currently no formal relationship between the Class namespace and the Class structure which are independently named. Since consistency would require support at the API level any GUI-level approach would not solve all use cases. We would like to formalize the relationship although we have not yet decided on an approach.

[ghidra1]

A temporary solution for GUI environments may be to have a plugin listening for Class or Structure name changes and mirror the changes as necessary after confirming with user.

[goatshriek]

Okay, thanks for the info! I will see about incorporating that functionality into my own plugin for my own workflows.

Regarding the different behavior in renaming the same structure from the Data Type Manager vs. the Structure Editor, is this an expected artifact of the lack of tie between these two data types? The fact that reloading the Code Browser always matches the behavior seen by the structure editor change makes me think that the Data Type Manager update doesn't fire some sort of update event properly. I've dug through both code paths, but haven't been able to identify exactly what is/isn't happening to cause the inconsistency.

[ghidra1]

@goatshriek, I don't follow what you mean. Renaming a structure from the Data Type tree appears to behave the same as renaming within the Structure Editor. Can you identify the scenarios where you see the behavior issue/difference.

[goatshriek]

Sure thing, here are some screenshots that make it a little more clear.

Via the Structure Editor

Before I click the save icon, things look like this with a simple decompilation, shown below. This is before step 3 in the reproduction steps.

Immediately after I click the save icon in the structure editor, the decompilation breaks as shown below. This is because the structure and the class name no longer match. I believe this is the expected behavior. This is step 4 in the reproduction steps.

Via the Data Type Manager

The state before I enter a new name and press enter, with the same decompilation as we saw before. I'll use a different name just to avoid any possibilities of the previous name somehow existing from the last test. This is step 6 in the reproduction steps.

After I press enter. The decompilation is updated with the new structure name! I must admit this made me happy that I had somehow found a way to cheat the system. This is step 8 in the reproduction steps.

Alas, after saving and re-loading the CodeBrowser the decompilation is actually broken due to the name mismatch (again shown below). This reinforces the idea that the structure editor behavior is the expected one. This is step 10 in the reproduction steps.

[Wall-AF]

The current workaround is to then rename the actual class as well from within the Symbol Tree view.

[goatshriek]

Yes, this workaround works for me and is what I've been doing. However, there are some nuances to it that I think highlight the weirdness I'm seeing.

For example, if I rename the structure via the Structure Editor (breaking the decompilation as expected), and then change the class name in the Symbol Tree to match, the decompilation remains broken until I restart the CodeBrowser. The screenshot below shows this state, before I restart:

And then after I save and restart:

So, for anyone looking to apply that workaround, make sure that you always rename classes and their structures via the Symbol Tree and Data Type Manager (I don't think order matters).

This is what makes me think there is a refresh event missing somewhere. It seems like the rename should immediately correct the resolution issue when done via the tree-based menus.

[ghidra1]

We confirmed that this class rename does present an issue for the this pointer data type on the cached function. We will look into correcting this. A possible work-around which would avoid closing the tool is to do an undo followed by a redo - not ideal but better than closing and re-opening the tool.

[goatshriek]

Ah, I did not know about the undo-redo trick, thanks! That is super handy.

[ghidra1]

Sure thing, here are some screenshots that make it a little more clear.

In the first case (structure editor) the rename is coupled with a complete save/update of the structure. This is triggering a dataTypeChanged which in turn is invalidating all functions. The change event triggers a re-decompile. Due to an issue with the decompiler and the this auto-param (separate issue currently being investigated) the class structure matching the class name can't be found and results in the error you are seeing. The auto-param will invent a non-DB class structure on the fly but decompiler lookup fails to find it.

In the second case (Via the Data Type Manager) a simple rename of the structure is done which does not currently invalidate any functions. A re-decompilation occurs which uses the cached function whose this auto-param refers to the old class structure although it has a new name. This happens because the function instance was not invalidated and is simply showing the new name of the cached this param. If it had been invalidated and re-decompiled the decompiler issue mentioned for the first case would have surfaced which would fail to find a class structure which matches the class name.

Two issues exist:

• function invalidation does not always occur
• decompiler currently has an issue with auto-generated non-DB class structure when a matching one is not found