feat(label): add ZGLabel and ZGDocumentLabelManager implementations #83

wesselvdv · 2023-01-22T18:51:07Z

This should at least resolve #66 and adds a starting point for #16.

No tests yet, as I think some discussion on the exact implementation is advisable.

I am not sure what the best approach is for solving #66. I see two possibilities with this PR:

Separate a ZGLabel and ZGVariable in the UI, and create separate tables for them to manage them.
A ZGLabel is referencing a ZGVariable, and as such the table implementation can be re-used for ZGLabel. When assigning a label to a variable, you would actually lift the referenced variable into a label and this means that the variable is moved from the variables list to the labels list. You can still interact with the variable in the same way, but it now is a label for all intents and purposes. (e.g. edit address formula and others)
Do not create separate tables for variables and labels, instead augment the variables view in some way that adds a simple but clear distinction between variables and variables with labels.
This would be the least amount of work, but I am not sure what the best way to indicate a label on a variable would be. An additional column is easiest, but is also a waste of space. (Assuming most variables don't have a label.) The creation of a label is the same as for 1. (e.g. lifting the ZGVariable into a ZGLabel). But it'll stick around in the variables array, and still be part of the variables table.

zorgiepoo · 2023-01-23T02:18:38Z

Thank you for the PR. Left some small comments for now but will take a closer look and think about this more.

wesselvdv · 2023-01-23T07:49:34Z

Thank you for the PR. Left some small comments for now but will take a closer look and think about this more.

I don't see any comments? Or am I stupid.

zorgiepoo · 2023-01-23T07:51:57Z

They should show up as inline comments here on GitHub's review.

wesselvdv · 2023-01-23T08:17:41Z

I don't see any review, maybe it's still pending? As in, did you confirm the review? Not sure if that's required or not. But I just did a test review, and I do see those comments.

Bit Slicer/ZGLabel.m

Bit Slicer/ZGDocumentLabelManager.m

Bit Slicer/ZGPyDebugger.m

Bit Slicer/ZGLabel.h

Bit Slicer/ZGLabel.m

zorgiepoo · 2023-01-23T07:50:18Z

Bit Slicer/ZGDocumentTableController.m

@@ -201,12 +203,13 @@ - (BOOL)updateDynamicVariableAddress:(ZGVariable *)variable
 {
 	BOOL needsToReload = NO;
 	ZGDocumentWindowController *windowController = _windowController;
-	if (windowController != nil && variable.usesDynamicAddress && !variable.finishedEvaluatingDynamicAddress)
+	if (windowController != nil && (variable.usesDynamicLabelAddress || variable.usesDynamicAddress) && !variable.finishedEvaluatingDynamicAddress)


I think there is an edge case. If a label uses a dynamic pointer address, a variable that references this label is basically also potentially-changing, but the variable might think its address can't change and won't update it..

Oh, yes. That's definitely an interesting case. That can be solved if we assume that usesDynamicAddress also included the usage of label(''). Which makes sense, as it is dynamic.

EDIT:

Wait, come to think of it. I don't think that case is valid. Currently all labels are updated before any variable is updated. So once the variables that are actually pointing to this label are being updated, they already have access to the most recent address.

The potential problem is if the label's address changes and the finishedEvaluatingDynamicAddress bit here. Once a variable's address is computed it's cached, unless this code detects it uses a pointer in the address formula. One (easy) way of fixing it (although not as 'efficient') is making any variable that references a label not use a cache so the address would have to be computed each time. Also the cache of all variables may need to be invalidated if a label is mutated or a label is removed or added or what not (if a cache is used).

Or maybe forbid a label from using a pointer.. but I don't know if that is a good idea. Relatedly I'm not sure if labels should be allowed to reference other labels.

I don't think we should forbid a label from using pointers. I also don't think we should forbid labels referencing other labels. That's a killer feature in my opinion.

I suggest we start tracking the dependant variables for each label. (e.g. add NSMutableArray * dependants), and use this information to invalidate the dependant variable cache when the label has changed. Depending on whether or not the variable who was invalidated is actually visible, it'll be updated. If it's not visible, then the cache is still invalid but it won't be calculated until it is.

For now if you want to make things easy I think we can disable caching when a variable uses a label; it's an optimization problem, and maybe very few variables will use labels in practice. But we can improve it later.

I'm not sure I'm following the idea of storing more state for dependents..

But another idea: when we create a new label we should be able to know if a pointer is referenced or if it references another label that references a pointer. When we parse a variable's address formula, we should be able to know what labels are referenced. If any of those labels reference pointers directly/indirectly we need to disable caching. The only additional state on a label is "does it use pointers?" But again, this may not be a problem we have to solve right now.

Yes, I think it might be best to solve this problem when it stops being a hypothetical one. I’ll change it so variable caching is disabled once a label is referenced.

Need to change:

if (error == nil && !variable.usesDynamicPointerAddress)

to:

if (error == nil && !variable.usesDynamicPointerAddress && !variable.usesDynamicLabelAddress)

I think.

Also in currentProcessChangedWithOldProcess:newProcess: need to set .finishedEvaluatingDynamicAddress = NO for all label variables.

zorgiepoo · 2023-01-28T16:16:49Z

Bit Slicer/ZGCalculator.m

+				ZGLabel *label = [documentLabelManager.labels objectForKey:labelString];
+
+				labelAddressNumber = @(label.address);
+				if (labelAddressNumber == nil)


I think if label is nil, no need to assign labelAddressNumber (leave it zero). Otherwise if it's not nil, assign to @(label.address) but no need to check the result is nil (this shouldn't happen).

I suppose you could return an error instead if the label doesn't exist.. but I don't think this is really an "error", just waiting until the label gets assigned.. (edit: I'm trying to think if it makes any difference from a caching perspective but I don't think so at the moment.. 🤔)

zorgiepoo · 2023-01-28T16:20:04Z

Bit Slicer/ZGDocument.m

@@ -186,6 +193,16 @@ - (BOOL)readFromFileWrapper:(NSFileWrapper *)fileWrapper ofType:(NSString *)__un
 		_data.variables = [NSArray array];
 	}

+	NSArray<ZGLabel *> *newLabels = [keyedUnarchiver decodeObjectOfClass:[NSArray class] forKey:ZGLabelsArrayKey];


This is probably wrong for specifying the decoded class but so is the code above it saving the variables. I can look at this later independently from this pull request.

zorgiepoo · 2023-01-28T16:20:54Z

Bit Slicer/ZGDocumentLabelManager.m

+
+		for (ZGLabel *label in documentData.labels)
+		{
+			if(label.name != nil)


Shouldn't be nil.

zorgiepoo · 2023-01-28T16:22:03Z

Bit Slicer/ZGDocumentLabelManager.m

+	NSMutableArray<ZGLabel *> *temporaryArray = [[NSMutableArray alloc] initWithArray:_documentData.labels];
+	[temporaryArray addObject:label];
+
+	_documentData.labels = [NSArray arrayWithArray:temporaryArray];


nit:

_documentData.labels = [_documentData.labels arrayByAddingObject:label];

zorgiepoo · 2023-01-28T16:27:42Z

Bit Slicer/ZGLabel.m

+		ZGVariable *_variable = [coder decodeObjectOfClass:[ZGVariable class] forKey:ZGVariableKey];
+
+		if(_name == NULL || _variable == NULL) {
+			return NULL;


These should use nil not NULL for obj-c types.

Also these local variables should not be prefixed with _ and you need to assign them to the ivars.

zorgiepoo · 2023-01-28T16:28:05Z

Bit Slicer/ZGLabel.m

+	{
+		_name = name;
+		_variable = [[ZGVariable alloc] initWithValue:NULL size:pointerSize address:address type:ZGPointer qualifier:qualifier pointerSize:pointerSize];
+	}


missing return self.

zorgiepoo · 2023-01-28T16:28:10Z

Bit Slicer/ZGLabel.m

+		_variable = [[ZGVariable alloc] initWithValue:NULL size:pointerSize address:variable.address type:ZGPointer qualifier:variable.qualifier pointerSize:pointerSize];
+
+		[_variable setAddressFormula:variable.addressFormula];
+	}


missing return self.

Also unused method for now (I rather add methods when needed than keep them around).

zorgiepoo · 2023-01-28T17:50:18Z

Bit Slicer/ZGPyDebugger.m

+	ZGMemoryAddress memoryAddress = 0x0;
+	char *identifier = NULL;
+
+	if (PyArg_ParseTuple(args, "sK:addLabel", &identifier, &memoryAddress))


This function should likely allow passing a numerical OR a string which would be creating a label with an address formula. It should be possible from scripting to create such dynamic labels.

I'm also not sure if there should be addLabel/removeLabel rather than setLabel with None being to remove the label 🤔

zorgiepoo · 2023-01-29T07:41:45Z

After some thinking, I think a variable needs to have a label in order for that label to be around. Basically a label needs to be attached to some variable in the table.

From a user point of view (and not implementation point of view): when a user wants to set a label onto a variable, that should just label the variable with a specific reference-able name. When the user edits the address formula of that variable, the description there (that explains base() and []s) can maybe indicate the reference-able label name of the variable (I believe I had a branch somewhere that actually did something like this). (Edit: the variable description can be updated too if the user hasn’t edited the description themselves yet and it has been so far auto-generated). Also changing the address formula of a variable that is labeled should update the variable/label appropriately. Any other variable can reference the label using label("myLabelName"). When a variable that has a label is removed, that label should also be discarded (unless maybe there's another variable with the same label like a copy but I’m not sure this should be allowed. There is undo support to think about too.). The only labels that need to be updated/managed are labels that are assigned to variables in the table.

From scripting, the user should only be able to set/change an existing label, not add or remove labels.

I don't want an ordered set of labels view the user needs to manage if I can avoid it.

Is it necessary that a label creates another variable? Maybe not. Is it necessary for the document to save another array of labels, perhaps not either.

wesselvdv · 2023-01-29T19:29:31Z

Interesting, that was my initial approach (e.g. label property on ZGVariable). You'd still need the ZGLabelManager to keep track of labels that are floating around as a lookup table between label ~> ZGVariable. Regarding the duplicates, you can detect these easily with the ZGLabelManager as it's being stored in a dict there. The UI would have to be added to facilitate the creation of a label then. Another option in the right-click menu perhaps?

zorgiepoo · 2023-01-30T00:26:32Z

Regarding copies, I don't think two variables currently in the table should be allowed to be assigned to the same label. So copying a variable that is labeled should likely strip the label. Trying to set the same label onto another variable should fail.

Probably a Edit Variable Label… menu item in the Variable menu from the menu bar and a contextual (control/right click) menu is needed.

zorgiepoo · 2024-05-06T03:11:32Z

I merged an implementation for adding labels in #95 (more details there). This PR helped me speed up writing an initial implementation.

wesselvdv force-pushed the feat/labels branch 2 times, most recently from 660abbb to b9e58df Compare January 22, 2023 19:00

zorgiepoo requested changes Jan 23, 2023

View reviewed changes

wesselvdv force-pushed the feat/labels branch 3 times, most recently from f0c6c20 to 492e076 Compare January 23, 2023 19:29

feat(label): add ZGLabel and ZGDocumentLabelManager implementations

740ba28

wesselvdv force-pushed the feat/labels branch from 492e076 to 740ba28 Compare January 24, 2023 08:35

zorgiepoo reviewed Jan 28, 2023

View reviewed changes

Bit Slicer/ZGDocumentLabelManager.m

for (ZGLabel *label in documentData.labels)

{

if(label.name != nil)

Copy link

Owner

zorgiepoo Jan 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't be nil.

zorgiepoo reviewed Jan 28, 2023

View reviewed changes

zorgiepoo closed this May 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(label): add ZGLabel and ZGDocumentLabelManager implementations #83

feat(label): add ZGLabel and ZGDocumentLabelManager implementations #83

wesselvdv commented Jan 22, 2023 •

edited

Loading

zorgiepoo commented Jan 23, 2023

wesselvdv commented Jan 23, 2023

zorgiepoo commented Jan 23, 2023

wesselvdv commented Jan 23, 2023 •

edited

Loading

zorgiepoo Jan 23, 2023

wesselvdv Jan 23, 2023 •

edited

Loading

zorgiepoo Jan 24, 2023 •

edited

Loading

zorgiepoo Jan 24, 2023

wesselvdv Jan 24, 2023 •

edited

Loading

zorgiepoo Jan 26, 2023

wesselvdv Jan 26, 2023

zorgiepoo Jan 28, 2023 •

edited

Loading

zorgiepoo Jan 28, 2023 •

edited

Loading

zorgiepoo Jan 28, 2023 •

edited

Loading

zorgiepoo Jan 28, 2023

zorgiepoo Jan 28, 2023 •

edited

Loading

zorgiepoo Jan 28, 2023

zorgiepoo Jan 28, 2023

zorgiepoo Jan 28, 2023

zorgiepoo Jan 28, 2023

zorgiepoo Jan 28, 2023 •

edited

Loading

zorgiepoo Jan 28, 2023

zorgiepoo Jan 28, 2023

zorgiepoo commented Jan 29, 2023 •

edited

Loading

wesselvdv commented Jan 29, 2023

zorgiepoo commented Jan 30, 2023

zorgiepoo commented May 6, 2024

feat(label): add ZGLabel and ZGDocumentLabelManager implementations #83

feat(label): add ZGLabel and ZGDocumentLabelManager implementations #83

Conversation

wesselvdv commented Jan 22, 2023 • edited Loading

zorgiepoo commented Jan 23, 2023

wesselvdv commented Jan 23, 2023

zorgiepoo commented Jan 23, 2023

wesselvdv commented Jan 23, 2023 • edited Loading

Choose a reason for hiding this comment

wesselvdv Jan 23, 2023 • edited Loading

Choose a reason for hiding this comment

zorgiepoo Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wesselvdv Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zorgiepoo Jan 28, 2023 • edited Loading

Choose a reason for hiding this comment

zorgiepoo Jan 28, 2023 • edited Loading

Choose a reason for hiding this comment

zorgiepoo Jan 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zorgiepoo Jan 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zorgiepoo Jan 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zorgiepoo commented Jan 29, 2023 • edited Loading

wesselvdv commented Jan 29, 2023

zorgiepoo commented Jan 30, 2023

zorgiepoo commented May 6, 2024

wesselvdv commented Jan 22, 2023 •

edited

Loading

wesselvdv commented Jan 23, 2023 •

edited

Loading

wesselvdv Jan 23, 2023 •

edited

Loading

zorgiepoo Jan 24, 2023 •

edited

Loading

wesselvdv Jan 24, 2023 •

edited

Loading

zorgiepoo Jan 28, 2023 •

edited

Loading

zorgiepoo Jan 28, 2023 •

edited

Loading

zorgiepoo Jan 28, 2023 •

edited

Loading

zorgiepoo Jan 28, 2023 •

edited

Loading

zorgiepoo Jan 28, 2023 •

edited

Loading

zorgiepoo commented Jan 29, 2023 •

edited

Loading