Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make memory address padding configurable #89

Merged
merged 4 commits into from
Mar 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,10 @@
"description": "Default words per group"
},
"memory-inspector.groupings.groupsPerRow": {
"type": ["string", "number"],
"type": [
"string",
"number"
],
"enum": [
"Autofit",
1,
Expand Down Expand Up @@ -220,6 +223,23 @@
],
"description": "Behavior when adding more memory beyond the current view."
},
"memory-inspector.addressPadding": {
"type": "string",
"enum": [
"Minimal",
"Unpadded",
"32bit",
"64bit"
],
"default": "Minimal",
"enumDescriptions": [
"Pads to largest address width in loaded range.",
"Disables padding.",
"Pads to 32 bits.",
"Pads to 64 bits."
],
"description": "Controls the padding with 0s for memory addresses, enhancing readability by aligning them within a specified bit-width context."
},
"memory-inspector.addressRadix": {
"type": "number",
"enum": [
Expand Down
7 changes: 5 additions & 2 deletions src/common/memory-range.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,14 @@ export function getRadixMarker(radix: Radix): string {
return radixPrefixMap[radix];
}

export function getAddressString(address: bigint, radix: Radix, architecture: Architecture = 32, addPadding = false): string {
const paddedLength = addPadding ? Math.ceil(architecture / Math.log2(radix)) : 0;
export function getAddressString(address: bigint, radix: Radix, paddedLength: number = 0): string {
return address.toString(radix).padStart(paddedLength, '0');
}

export function getAddressLength(padding: number, radix: Radix): number {
return Math.ceil(padding / Math.log2(radix));
}

export function toHexStringWithRadixMarker(target: bigint): string {
return `${getRadixMarker(Radix.Hexadecimal)}${getAddressString(target, Radix.Hexadecimal)}`;
}
Expand Down
2 changes: 2 additions & 0 deletions src/plugin/manifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ export const DEFAULT_GROUPS_PER_ROW: GroupsPerRowOption = 4;
// Scroll
export const CONFIG_SCROLLING_BEHAVIOR = 'scrollingBehavior';
export const DEFAULT_SCROLLING_BEHAVIOR = 'Paginate';
export const CONFIG_ADDRESS_PADDING = 'addressPadding';
export const DEFAULT_ADDRESS_PADDING = 'Minimal';
export const CONFIG_ADDRESS_RADIX = 'addressRadix';
export const DEFAULT_ADDRESS_RADIX = 16;
export const CONFIG_SHOW_RADIX_PREFIX = 'showRadixPrefix';
Expand Down
5 changes: 3 additions & 2 deletions src/plugin/memory-webview-main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ import {
import { MemoryProvider } from './memory-provider';
import { outputChannelLogger } from './logger';
import { VariableRange } from '../common/memory-range';
import { MemoryViewSettings, ScrollingBehavior } from '../webview/utils/view-types';
import { AddressPaddingOptions, MemoryViewSettings, ScrollingBehavior } from '../webview/utils/view-types';

interface Variable {
name: string;
Expand Down Expand Up @@ -225,9 +225,10 @@ export class MemoryWebview implements vscode.CustomReadonlyEditorProvider {
const visibleColumns = CONFIGURABLE_COLUMNS
.filter(column => vscode.workspace.getConfiguration(manifest.PACKAGE_NAME).get<boolean>(column, false))
.map(columnId => columnId.replace('columns.', ''));
const addressPadding = AddressPaddingOptions[memoryInspectorConfiguration.get(manifest.CONFIG_ADDRESS_PADDING, manifest.DEFAULT_ADDRESS_PADDING)];
colin-grant-work marked this conversation as resolved.
Show resolved Hide resolved
const addressRadix = memoryInspectorConfiguration.get<number>(manifest.CONFIG_ADDRESS_RADIX, manifest.DEFAULT_ADDRESS_RADIX);
const showRadixPrefix = memoryInspectorConfiguration.get<boolean>(manifest.CONFIG_SHOW_RADIX_PREFIX, manifest.DEFAULT_SHOW_RADIX_PREFIX);
return { title, bytesPerWord, wordsPerGroup, groupsPerRow, scrollingBehavior, visibleColumns, addressRadix, showRadixPrefix };
return { title, bytesPerWord, wordsPerGroup, groupsPerRow, scrollingBehavior, visibleColumns, addressPadding, addressRadix, showRadixPrefix };
}

protected async readMemory(request: DebugProtocol.ReadMemoryArguments): Promise<MemoryReadResult> {
Expand Down
8 changes: 4 additions & 4 deletions src/webview/columns/address-column.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@

import React, { ReactNode } from 'react';
import { BigIntMemoryRange, getAddressString, getRadixMarker } from '../../common/memory-range';
import { ColumnContribution, ColumnFittingType } from './column-contribution-service';
import { Memory, MemoryDisplayConfiguration } from '../utils/view-types';
import { ColumnContribution, ColumnFittingType, TableRenderOptions } from './column-contribution-service';
import { Memory } from '../utils/view-types';

export class AddressColumn implements ColumnContribution {
static ID = 'address';
Expand All @@ -28,10 +28,10 @@ export class AddressColumn implements ColumnContribution {

fittingType: ColumnFittingType = 'content-width';

render(range: BigIntMemoryRange, _: Memory, options: MemoryDisplayConfiguration): ReactNode {
render(range: BigIntMemoryRange, _: Memory, options: TableRenderOptions): ReactNode {
return <span className='memory-start-address'>
{options.showRadixPrefix && <span className='radix-prefix'>{getRadixMarker(options.addressRadix)}</span>}
<span className='address'>{getAddressString(range.startAddress, options.addressRadix)}</span>
<span className='address'>{getAddressString(range.startAddress, options.addressRadix, options.effectiveAddressLength)}</span>
</span>;
}
}
1 change: 1 addition & 0 deletions src/webview/components/memory-table.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ interface MemoryTableProps extends TableRenderOptions, MemoryDisplayConfiguratio
decorations: Decoration[];
offset: number;
count: number;
effectiveAddressLength: number;
fetchMemory(partialOptions?: Partial<DebugProtocol.ReadMemoryArguments>): Promise<void>;
isMemoryFetching: boolean;
isFrozen: boolean;
Expand Down
4 changes: 4 additions & 0 deletions src/webview/components/memory-widget.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ interface MemoryWidgetProps extends MemoryDisplayConfiguration {
memoryReference: string;
offset: number;
count: number;
effectiveAddressLength: number;
isMemoryFetching: boolean;
refreshMemory: () => void;
updateMemoryArguments: (memoryArguments: Partial<DebugProtocol.ReadMemoryArguments>) => void;
Expand Down Expand Up @@ -72,6 +73,7 @@ export class MemoryWidget extends React.Component<MemoryWidgetProps, MemoryWidge
updateRenderOptions={this.props.updateMemoryDisplayConfiguration}
resetRenderOptions={this.props.resetMemoryDisplayConfiguration}
refreshMemory={this.props.refreshMemory}
addressPadding={this.props.addressPadding}
addressRadix={this.props.addressRadix}
showRadixPrefix={this.props.showRadixPrefix}
toggleColumn={this.props.toggleColumn}
Expand All @@ -88,9 +90,11 @@ export class MemoryWidget extends React.Component<MemoryWidgetProps, MemoryWidge
groupsPerRow={this.props.groupsPerRow}
offset={this.props.offset}
count={this.props.count}
effectiveAddressLength={this.props.effectiveAddressLength}
fetchMemory={this.props.fetchMemory}
isMemoryFetching={this.props.isMemoryFetching}
scrollingBehavior={this.props.scrollingBehavior}
addressPadding={this.props.addressPadding}
Copy link
Contributor

Choose a reason for hiding this comment

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

I have some misgivings about the number of places we're passing around the new memoized padding / length values when they're relevant to only one column, and we should avoid knowing too much about column internals outside of those contributions.

One idea about how to handle it would be to introduce a new lifecycle on the columns so that they can update any relevant state onComponentDidUpdate or whatever seems best. Another (obviously beyond the scope of this PR) would be to use something other than React so that individual columns could have tighter control of their own rendering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean -- conceptually -- onComponentDidUpdate in App, we invoke an update on each column, passing them a new object state to avoid having to pass column-specific state down to the MemoryWidget/MemoryTable?
That sounds like a good idea to me!
I feel though such a change probably deserves a separate PR, wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean -- conceptually -- onComponentDidUpdate in App, we invoke an update on each column, passing them a new object state to avoid having to pass column-specific state down to the MemoryWidget/MemoryTable?

Yes, exactly. That way columns are kept up-to-date on changes in props/state and can store the results of any calculations they'd otherwise repeat.

I feel though such a change probably deserves a separate PR, wdyt?

I'd be inclined to do it here because this PR is introducing a new prop that's being passed in a style we might like to avoid, but if you feel strongly, we could put it off.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be frank: I agree with @planger that this should be addressed in a separate refactoring PR.
It is a great idea to have more fine granular control over updates. In the sense of avoiding to flood components with "unsed" or just passed through props. But it sounds like this is something that should be properly analyzed and rolled out across the board. I am sure there is more potential for this beyond the address padding. And a separate PR will give clearer separation of the feature enhancement and refactoring work.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with that approach, though I think it's not generally a good idea to acknowledge that something is not the direction we want to go and then move forward with it anyway. I've approved the PR - @planger, feel free to make whatever changes to the wording of the preference enum descriptions that you prefer and merge at your convenience.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, @colin-grant-work ! I appreciate that the situation isn't ideal. Let's make sure that we address this as soon as we can.

addressRadix={this.props.addressRadix}
showRadixPrefix={this.props.showRadixPrefix}
isFrozen={this.props.isFrozen}
Expand Down
24 changes: 20 additions & 4 deletions src/webview/components/options-widget.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,14 @@ import { OverlayPanel } from 'primereact/overlaypanel';
import { classNames } from 'primereact/utils';
import React, { FocusEventHandler, KeyboardEvent, KeyboardEventHandler, MouseEventHandler } from 'react';
import { TableRenderOptions } from '../columns/column-contribution-service';
import {
SerializedTableRenderOptions,
} from '../utils/view-types';
import { AddressPaddingOptions, SerializedTableRenderOptions } from '../utils/view-types';
import { MultiSelectWithLabel } from './multi-select';
import { CONFIG_BYTES_PER_WORD_CHOICES, CONFIG_GROUPS_PER_ROW_CHOICES, CONFIG_WORDS_PER_GROUP_CHOICES } from '../../plugin/manifest';
import { tryToNumber } from '../../common/typescript';
import { Checkbox } from 'primereact/checkbox';

export interface OptionsWidgetProps
extends Omit<TableRenderOptions, 'scrollingBehavior'>,
extends Omit<TableRenderOptions, 'scrollingBehavior' | 'effectiveAddressLength'>,
Required<DebugProtocol.ReadMemoryArguments> {
title: string;
updateRenderOptions: (options: Partial<SerializedTableRenderOptions>) => void;
Expand All @@ -58,6 +56,7 @@ const enum InputId {
BytesPerWord = 'word-size',
WordsPerGroup = 'words-per-group',
GroupsPerRow = 'groups-per-row',
AddressPadding = 'address-padding',
AddressRadix = 'address-radix',
ShowRadixPrefix = 'show-radix-prefix',
}
Expand Down Expand Up @@ -321,6 +320,20 @@ export class OptionsWidget extends React.Component<OptionsWidgetProps, OptionsWi
className='advanced-options-dropdown' />

<h2>Address Format</h2>

<label
htmlFor={InputId.AddressPadding}
className='advanced-options-label'
>
Address Padding
</label>
<Dropdown
id={InputId.AddressPadding}
value={this.props.addressPadding}
onChange={this.handleAdvancedOptionsDropdownChange}
options={Object.entries(AddressPaddingOptions).map(([label, value]) => ({ label, value }))}
className="advanced-options-dropdown" />

<label
htmlFor={InputId.AddressRadix}
className='advanced-options-label'
Expand Down Expand Up @@ -416,6 +429,9 @@ export class OptionsWidget extends React.Component<OptionsWidgetProps, OptionsWi
case InputId.GroupsPerRow:
this.props.updateRenderOptions({ groupsPerRow: tryToNumber(value) ?? value });
break;
case InputId.AddressPadding:
this.props.updateRenderOptions({ addressPadding: value });
break;
case InputId.AddressRadix:
this.props.updateRenderOptions({ addressRadix: Number(value) });
break;
Expand Down
37 changes: 36 additions & 1 deletion src/webview/memory-webview-view.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,11 @@ import { AddressColumn } from './columns/address-column';
import { DataColumn } from './columns/data-column';
import { PrimeReactProvider } from 'primereact/api';
import 'primeflex/primeflex.css';
import { getAddressLength, getAddressString } from '../common/memory-range';

export interface MemoryAppState extends MemoryState, MemoryDisplayConfiguration {
title: string;
effectiveAddressLength: number;
decorations: Decoration[];
columns: ColumnStatus[];
isFrozen: boolean;
Expand All @@ -51,6 +53,7 @@ const MEMORY_DISPLAY_CONFIGURATION_DEFAULTS: MemoryDisplayConfiguration = {
wordsPerGroup: 1,
groupsPerRow: 4,
scrollingBehavior: 'Paginate',
addressPadding: 'Min',
addressRadix: 16,
showRadixPrefix: true,
};
Expand All @@ -70,6 +73,7 @@ class App extends React.Component<{}, MemoryAppState> {
memoryReference: '',
offset: 0,
count: 256,
effectiveAddressLength: 0,
decorations: [],
columns: columnContributionService.getColumns(),
isMemoryFetching: false,
Expand All @@ -91,6 +95,18 @@ class App extends React.Component<{}, MemoryAppState> {
messenger.sendNotification(readyType, HOST_EXTENSION, undefined);
}

public componentDidUpdate(_: {}, prevState: MemoryAppState): void {
const addressPaddingNeedsUpdate =
(this.state.addressPadding === 'Min' && this.state.memory !== prevState.memory)
|| this.state.addressPadding !== prevState.addressPadding;
if (addressPaddingNeedsUpdate) {
const effectiveAddressLength = this.getEffectiveAddressLength(this.state.memory);
if (this.state.effectiveAddressLength !== effectiveAddressLength) {
this.setState({ effectiveAddressLength });
}
}
}

public render(): React.ReactNode {
return <PrimeReactProvider>
<MemoryWidget
Expand All @@ -101,6 +117,7 @@ class App extends React.Component<{}, MemoryAppState> {
offset={this.state.offset ?? 0}
count={this.state.count}
title={this.state.title}
effectiveAddressLength={this.state.effectiveAddressLength}
updateMemoryArguments={this.updateMemoryState}
updateMemoryDisplayConfiguration={this.updateMemoryDisplayConfiguration}
resetMemoryDisplayConfiguration={this.resetMemoryDisplayConfiguration}
Expand All @@ -115,6 +132,7 @@ class App extends React.Component<{}, MemoryAppState> {
groupsPerRow={this.state.groupsPerRow}
wordsPerGroup={this.state.wordsPerGroup}
scrollingBehavior={this.state.scrollingBehavior}
addressPadding={this.state.addressPadding}
addressRadix={this.state.addressRadix}
showRadixPrefix={this.state.showRadixPrefix}
/>
Expand Down Expand Up @@ -156,9 +174,11 @@ class App extends React.Component<{}, MemoryAppState> {
executor => executor.fetchData(completeOptions)
));

const memory = this.convertMemory(response);

this.setState({
decorations: decorationService.decorations,
memory: this.convertMemory(response),
memory,
memoryReference: completeOptions.memoryReference,
offset: completeOptions.offset,
count: completeOptions.count,
Expand All @@ -179,6 +199,21 @@ class App extends React.Component<{}, MemoryAppState> {
return { bytes, address };
}

protected getEffectiveAddressLength(memory?: Memory): number {
const { addressRadix, addressPadding } = this.state;
return addressPadding === 'Min' ? this.getLastAddressLength(memory) : getAddressLength(addressPadding, addressRadix);
}

protected getLastAddressLength(memory?: Memory): number {
if (memory === undefined || this.state.groupsPerRow === 'Autofit') {
return 0;
}
const rowLength = this.state.bytesPerWord * this.state.wordsPerGroup * this.state.groupsPerRow;
const rows = Math.ceil(memory.bytes.length / rowLength);
const finalAddress = memory.address + BigInt(((rows - 1) * rowLength));
return getAddressString(finalAddress, this.state.addressRadix).length;
}

protected toggleColumn = (id: string, active: boolean): void => { this.doToggleColumn(id, active); };
protected async doToggleColumn(id: string, isVisible: boolean): Promise<void> {
const columns = isVisible ? await columnContributionService.show(id, this.state) : columnContributionService.hide(id);
Expand Down
11 changes: 10 additions & 1 deletion src/webview/utils/view-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export interface Memory {
export interface SerializedTableRenderOptions extends MemoryDisplayConfiguration {
columnOptions: Array<{ label: string, doRender: boolean }>;
endianness: Endianness;
bytesPerWord: number;
effectiveAddressLength: number;
}

export interface Event<T> {
Expand Down Expand Up @@ -84,11 +84,20 @@ export interface MemoryDisplayConfiguration {
wordsPerGroup: number;
groupsPerRow: GroupsPerRowOption;
scrollingBehavior: ScrollingBehavior;
addressPadding: AddressPadding;
addressRadix: Radix;
showRadixPrefix: boolean;
}
export type ScrollingBehavior = 'Paginate' | 'Infinite';

export type AddressPadding = 'Min' | number;
export const AddressPaddingOptions = {
'Minimal': 'Min',
'Unpadded': 0,
'32bit': 32,
'64bit': 64,
} as const;

export interface ColumnVisibilityStatus {
visibleColumns: string[];
}
Expand Down
Loading