Disassembly view #125737

xisui-MSFT · 2021-06-08T10:25:36Z

This PR fixes #124163

src/vs/workbench/contrib/debug/test/browser/mockDebug.ts

isidorn · 2021-06-08T11:57:26Z

src/vs/workbench/contrib/debug/common/debugSchemas.ts

+}
+
+// disassembly extension point
+export const disassemblyExtPoint = extensionsRegistry.ExtensionsRegistry.registerExtensionPoint<IDisassemblyContribution[]>({


Do we really need a new contribution point for this? I think not, and all of this can be achieved using the capability supportsDisassembleRequest.
So I suggest to remove this.

fyi @weinand

@xisui-MSFT what's the purpose of this contribution point?

My understanding is that supportsDisassembleRequest is set when the debugger supports disassemble request? However, we don't want the "show disassembly"/"go to disassembly" option to exist in files other than those of certain languages.

Ultimately, our goal is to achieve similar functions as in VS. I.e., when right clicking a certain line in source (may not be %rip), go to disassembly corresponds to that line.

@xisui-MSFT Thanks, now I understand the problem you are addressing with the contribution point.

I think for now it is OK to introduce an (internal) contribution point.
But since a general solution will involve public extension API, we'll have to consider more scenarios in different debuggers before we can create a "proposed API" (that will have to go through the regular VS Code API process).

Alternative idea: let's not introduce a new contribution point but instead use the languages contribution point which debug extension already use to say they are interested in some langauges. Mock debug example.

So the logic would be, if there is an active debug session which supportsDisassembleRequest then show the action for all editor language types that are specified under the languages contribution of that debug extension.

What do you think?

CONTEXT_DISASSEMBLE_REQUEST_SUPPORTED shuold be set here
based on the focused session

For the new ext point as we discussed above this is not needed for step one, simply use the debuggers.languages contribution point for now.

I'm fine with either way... My point is that looks like we can't use IDebugService in the precondition of the action, but seems like IDebugService is the only way to access this ext point.

@xisui-MSFT define the precondition in common/debug.ts as all other preconditiions are defined. And they you will be able to access it from the action. And the debugViewModel will actually "implement" the precondition based on focused session.

I guess you meant to make it a context key just like CONTEXT_DISASSEMBLE_REQUEST_SUPPORTED? Although I can change debugAdapterManager to be able to access the ext point, I'm not sure how to get the language id of the current editor in it?

Using workbench contribution as we discussed offline.

src/vs/workbench/contrib/debug/common/debugModel.ts

isidorn · 2021-06-08T12:01:00Z

src/vs/workbench/contrib/debug/common/debug.ts

@@ -34,6 +34,7 @@ export const WATCH_VIEW_ID = 'workbench.debug.watchExpressionsView';
 export const CALLSTACK_VIEW_ID = 'workbench.debug.callStackView';
 export const LOADED_SCRIPTS_VIEW_ID = 'workbench.debug.loadedScriptsView';
 export const BREAKPOINTS_VIEW_ID = 'workbench.debug.breakPointsView';
+export const DISASSEMBLY_VIEW_ID = 'workbench.debug.disassemblyView';


Looks good 👍

src/vs/workbench/contrib/debug/browser/rawDebugSession.ts

isidorn · 2021-06-08T12:11:27Z

src/vs/workbench/contrib/debug/browser/disassemblyView.ts

+	private static readonly NUM_INSTRUCTIONS_TO_LOAD = 50;
+
+	private _fontInfo: BareFontInfo;
+	private _disassembledInstructions: WorkbenchTable<IDisassembledInstructionEntry> | null;


My code style is to use undefined not null. So unless you have a strong reason to use null I suggest to go with undefined

Yeah I don't have a preference on this and I can change it.

Great, thanks

isidorn · 2021-06-08T12:12:51Z

src/vs/workbench/contrib/debug/browser/disassemblyView.ts

+			],
+			{
+				identityProvider: { getId: (e: IDisassembledInstructionEntry) => e.instruction.address },
+				horizontalScrolling: false,


Make sure to also have an accessibilityProvider

Will add a TODO to make sure I don't forget this. I wanted to add it after adding some proper rendering.

Great thanks. As for inspiration you can checkout some other accessibilityProviders in debug lang. Try to have concise but descriptive messages with the least important piece of info at the end.

isidorn · 2021-06-08T12:17:15Z

src/vs/workbench/contrib/debug/browser/disassemblyView.ts

+
+export class DisassemblyView extends EditorPane {
+
+	private static readonly NUM_INSTRUCTIONS_TO_LOAD = 50;


50 is a very low number here. The scroll bar will considerably jump as soon as the user scrolls a bit.
I suggest to use a much larger number like a 1000 and make sure to have a lazily model that is evaluated only once the user scrolls all the way up to it (the element gets rendered)

In fact we don't want this number to be too large. The code may run on remote devices than don't have a very large communication bandwidth. I agree we can re-evaluate this value later though.

The table scrolling will be very weird and the UI wont be smooth. It wont be a nice user experience.
If remote is the only concern I suggest to maybe even change this number depending if we are in a remote scenario.

I think the UI looks fine though. Either way it's only a one line change, and later we can setup a meeting to go through the end to end experience, in which we can fix all of these minor issues.

isidorn · 2021-06-08T12:18:06Z

src/vs/workbench/contrib/debug/browser/disassemblyView.ts

+			}
+		)) as WorkbenchTable<IDisassembledInstructionEntry>;
+
+		this.loadDisassembledInstructions(0, DisassemblyView.NUM_INSTRUCTIONS_TO_LOAD * 2, '0x00005000');


I find it weird that you called the constant NUM_INSTRUCTIONS_TO_LOAD but then you load twice as much.
I think the constant should be larger and then you would either use it or it divided by 2, if that makes sense?

I consider it as number of instructions to load each time. Since the current instruction is revealed in the middle of the viewport, the first load actually loads "twice": "first time" before the current instruction, and "second time" include and after the current instruction.

Okey. I was just sharing that I found it a bit confusing :)

isidorn · 2021-06-08T12:23:00Z

src/vs/workbench/contrib/debug/browser/disassemblyView.ts

+
+		this._disassembledInstructions.onDidScroll(e => {
+			if (e.oldScrollTop > e.scrollTop && e.scrollTop < e.height) {
+				const topElement = Math.floor(e.scrollTop / this._fontInfo.lineHeight) + DisassemblyView.NUM_INSTRUCTIONS_TO_LOAD;


Let's comment out this onDidScroll code so we try to find the issue with the reveal that you mentioned.
So if you comment this out and you just do

this.loadDisassembledInstructions(0, DisassemblyView.NUM_INSTRUCTIONS_TO_LOAD * 2, '0x00005000'); this._disassembledInstructions.reveal(DisassemblyView.NUM_INSTRUCTIONS_TO_LOAD, 0.5);

You still get an issue? Nothing gets rendered?
How about if you comment out the reveal code do things get rendered then?

If I comment out those two lines, nothing is rendered.
If I comment out the reveal line only, the lines are rendered, and item 0 is at the top of the view.

Screen shot during debugging:

Immediately after creating the table:

Second time reveal is invoked (in scroll event listener):

Ok, thanks for providing more details.
To clarify:

You comment out your onDidScroll code and nothing gets rendered because you call reveal

You believe this is scrollTop is 0. But that is expected, the table is in the inital position and the top element is shown.

I just tried this out on my machine and it works just fine. I have simple relaxed the rule of the Go to Disassembly command to be always shown and the table gets nicely rendered for me (with no other changes to your source code)

Notice how the scroll bar is properly 70% through on the initial show. So I do not understand what the issue is on your side

If you still see the issue on your side I suggest that you simply make a change to the PR such that I can also reproduce the problem on my machine if possible and then I can investigate with Joao the owner of the table widget. However so far all looks good to me.

Sorry for the confusion. ScrollTop doesn't have problems. The problem is that List.view.renderHeight is 0.

Not sure why it's not reproing on your side though. Did you start debugging VS Code from VS Code, or are you using a packaged build?

I was running vscode out of source, not packaged.
I will check with Joao if he has ideas why it is broken on your machine and not on mine.
Maybe one of your colleagues could also try, just so we see if he also sees the problem on his or her machine?

Joao had a good idea:
"renderHeight = 0 usually means the client did not call layout()"

So make sure to call the layout method.
Once the table is layouted you should call reveal on it
Here's how the runtime extensions editor is doing it:

https://github.com/microsoft/vscode/blob/main/src/vs/workbench/contrib/extensions/browser/abstractRuntimeExtensionsEditor.ts#L458

Well that function was copied from your code... Felix said he couldn't repro the issue either. So it's probably something wrong with my machine... Let's worry about this later if any of us see this again.

Ok, just make sure to call layout please.
Also my code is 5 years old and has not been updated since. It needs a lot of improvements, it is just an example.

isidorn · 2021-06-08T12:24:53Z

@xisui-MSFT great work 👏 I left comments across various files, I hope you find them helpful.
As for the reveal issue I have commented in the code with some ideas how we can potentially investigate the issue in more details. Let me know once you tackle my comments and if you have some new findings regarding the reveal funcitonality.

fyi @lramos15 since this might be a good fit to merge one day with the hex editor. Both of them have a sort of an infinite virtualised editor view

xisui-MSFT · 2021-06-11T00:30:25Z

src/vs/workbench/contrib/debug/browser/disassemblyView.ts

+		if (element.allowBreakpoint) {
+			const breakpointIcon = 'codicon-' + icons.breakpoint.regular.id;
+			const breakpointHintIcon = 'codicon-' + icons.debugBreakpointHint.id;
+
+			if (element.isBreakpointSet) {
+				templateData.icon.classList.add(breakpointIcon);
+			}
+
+			templateData.container.onmouseover = () => {
+				templateData.icon.classList.add(breakpointHintIcon);
+			};
+
+			templateData.container.onmouseout = () => {
+				templateData.icon.classList.remove(breakpointHintIcon);
+			};
+
+			templateData.container.onclick = () => {
+				if (element.isBreakpointSet) {
+					element.isBreakpointSet = false;
+					templateData.icon.classList.remove(breakpointIcon);
+				} else if (element.allowBreakpoint) {
+					element.isBreakpointSet = true;
+					templateData.icon.classList.add(breakpointIcon);
+				}
+			};
+		}


@isidorn I think there should be a better way to render breakpoints, as the breakpoint HTMLElement in main editor seems to allow duplicate items in classList. Do you know if I missed anything?

Breakpoint in the main editor is rendered using editor glyph margins.
Unfortunately we do not have those for our custom editor, so we have to do something like this.

Just make sure to add all those listeners to a disposable list!

They are not disposable so I can't register them in the disposable list. But I set them to null in the dispose function, and I think this should be fine?

Please use addStandardDisposableListener
Code pointer https://github.com/microsoft/vscode/blob/main/src/vs/base/browser/dom.ts#L88

isidorn · 2021-06-11T09:07:28Z

fyi next week and the following Monday I will be on vacation 🌴

xisui-MSFT · 2021-06-16T00:01:48Z

src/vs/workbench/contrib/debug/browser/disassemblyView.ts

+		return { instruction };
+	}
+
+	renderElement(element: IDisassembledInstructionEntry, index: number, templateData: IInstructionColumnTemplateData, height: number | undefined): void {


@isidorn Looks like there may be problems rendering the rows if the content is very long. Even when allow horizontal scrolling, the initial width of the table seems to limit the length being rendered in a row.

Also, I didn't realize this issue before, but since the first column is breakpoints column, we don't want it disappear when scrolling horizontally. So is there a way to keep it fixed from horizontal scrolling?

Table should allow infinite width, you can just scroll to reach it. Can you please clarify what is the problem? You can not reach the end of the row? Are you sure you are properly calling layout?

I think it is currently not possible to have rows which are always visible in the table. I am double checking with Joao, but we can think about this as step 2 as an enhancement to the table.
Alternative approach would be to use the list instead of the table and then the whole row rendering is up to you so you can use css tricks to make sure that the breakpoint is always visible

I just got back from Joao, and horizontal scrolling is not supported by the table #117049

I was not aware of this, otherwise I would have brought it up sooner. So either we add horizontal scrolling some time in the future. Or we should consider to use the list instead and leave the rendering and positioning of fields to the implementation.

Thanks for checking! We think a table should be fine for the first iteration. Anyway, I tried list this afternoon but wasn't able to figure out how to fix an element from scrolling. Looks like the most commonly used CSS style for this is position: fixed, but this doesn't work in a list. Any suggestions?

@xisui-MSFT this would need some css magic. Your position:fixed sounds like a good idea. Though I am not sure if this will work horizontally.
The list should not prevent you from controlling your css layout. So it should be possible, if you ran out of ideas let me know and I can investigate online...

Actually I have already run out of ideas... I tried quite a few combinations of css styles but none of them work. Also the behavior seems to be different than when I tried to test with a simple webpage since position:fixed does work there.

Although looks like position:sticky may also be a good candidate, it doesn't work either. Feels like the concept of viewport with respect to entries in the list is different from that of the entire page...

Would be great if you could take a look. Thanks!

I checked with our css wizard and he said to try to go with position:sticky as you mentioned. He linked me this article, maybe it helps https://elad.medium.com/css-position-sticky-how-it-really-works-54cd01dc2d46

The only thing that wasn't so obvious that mentioned in this article is it requires a container to work. However, a list entry is already a container, so unfortunately it doesn't really help. Also I try to add another container under the list entry, and still no luck...

yuehuang010 · 2021-06-21T19:19:17Z

Hi @isidorn, at the moment, we have an early version of Disassembly View which support displaying disassembly and scrolling.
Breakpoints and Stepping are not working or partially implemented. Should we look into committing this PR as is or wait for more functionality to come online.
Thanks
Felix

isidorn · 2021-06-22T17:13:08Z

@yuehuang010 thanks a lot for the updates on this PR.
There has been great progress but I would prefer that we merge this PR once we polish this a bit more.

Ideally once we merge this in it would be in a good state such that we can together write a test plan item for this feature and we can test this thoroughly in our VS Code endgame and file issues so you can fix and improve the experience even more. That way we can get this to production quality.
If you are worried about diverging from main too much, I suggest that you simple merge with latest from main from time to time.

Let me know what you think about this plan and thanks for your great work.

isidorn · 2021-06-25T18:28:35Z

Chrome is doing a memory inspector, would be great if we try it out to learn about their approach
https://developer.chrome.com/blog/memory-inspector/

xisui-MSFT · 2021-06-25T21:26:31Z

@isidorn Thanks for sharing this. I looked at their approach, and seems like it's quite different from ours. One major difference is that *.wasm is a file that has a fixed size. And actually not being able to use a file with a fixed size is almost the only reason we decided that we shouldn't use the existing text editors in VS Code.

Feature wise, for assembly view only, I think we have achieved most features they have currently (while UI may need a lot of tweaks, and we can discuss them on Monday), and we are pursuing more features, such as debug tooltip.

For memory view and diagnostics view, I believe they are on our list but I'm not sure about the current progress.

jogo- · 2021-07-03T06:08:59Z

src/vs/workbench/contrib/debug/browser/debug.contribution.ts

@@ -362,6 +366,12 @@ viewsRegistry.registerViews([{ id: BREAKPOINTS_VIEW_ID, name: nls.localize('brea
 viewsRegistry.registerViews([{ id: WelcomeView.ID, name: WelcomeView.LABEL, containerIcon: icons.runViewIcon, ctorDescriptor: new SyncDescriptor(WelcomeView), order: 1, weight: 40, canToggleVisibility: true, when: CONTEXT_DEBUG_UX.isEqualTo('simple') }], viewContainer);
 viewsRegistry.registerViews([{ id: LOADED_SCRIPTS_VIEW_ID, name: nls.localize('loadedScripts', "Loaded Scripts"), containerIcon: icons.loadedScriptsViewIcon, ctorDescriptor: new SyncDescriptor(LoadedScriptsView), order: 35, weight: 5, canToggleVisibility: true, canMoveView: true, collapsed: true, when: ContextKeyExpr.and(CONTEXT_LOADED_SCRIPTS_SUPPORTED, CONTEXT_DEBUG_UX.isEqualTo('default')) }], viewContainer);

+// Register disassmebly editor


Typo: disassmebly -> disassembly

jogo- · 2021-07-03T06:11:29Z

src/vs/workbench/contrib/debug/browser/debugService.ts

+			const result: DebugProtocol.DisassembledInstruction[] = [];
+
+			for (let i = 0; i < instructionCount; i++) {
+				result.push({ address: `${i + offset}`, instruction: `debugger is not availible.` });


Typo: availible -> available

jogo- · 2021-07-03T06:18:13Z

src/vs/workbench/contrib/debug/common/debug.ts

@@ -430,6 +437,12 @@ export interface IDataBreakpoint extends IBaseBreakpoint {
 	readonly accessType: DebugProtocol.DataBreakpointAccessType;
 }

+export interface IInstructionBreakpoint extends IBaseBreakpoint {
+	// instructionReference is from debugProtocal and is address for purposes.


Typo: debugProtocal -> debugProtocol
"is address for purposes." ???

jogo- · 2021-07-03T06:22:35Z

src/vs/workbench/contrib/debug/browser/disassemblyView.ts

+	private _currentInstructionAddress: string | undefined;
+	private _disassembledInstructions: WorkbenchTable<IDisassembledInstructionEntry> | undefined;
+	private _onDidChangeStackFrame: Emitter<void>;
+	private _privousDebuggingState: State;


Typo: _privousDebuggingState -> _previousDebuggingState

jogo- · 2021-07-03T06:31:46Z

src/vs/workbench/contrib/debug/browser/disassemblyView.ts

+
+}
+
+export class DisassemblyVIewContribution implements IWorkbenchContribution {


Letter case: DisassemblyVIewContribution -> DisassemblyViewContribution

jogo- · 2021-07-03T06:41:47Z

src/vs/workbench/contrib/debug/common/debug.ts

@@ -80,6 +81,9 @@ export const CONTEXT_VARIABLE_EVALUATE_NAME_PRESENT = new RawContextKey<boolean>
 export const CONTEXT_EXCEPTION_WIDGET_VISIBLE = new RawContextKey<boolean>('exceptionWidgetVisible', false, { type: 'boolean', description: nls.localize('exceptionWidgetVisible', "True when the exception widget is visible.") });
 export const CONTEXT_MULTI_SESSION_REPL = new RawContextKey<boolean>('multiSessionRepl', false, { type: 'boolean', description: nls.localize('multiSessionRepl', "True when there is more than 1 debug console.") });
 export const CONTEXT_MULTI_SESSION_DEBUG = new RawContextKey<boolean>('multiSessionDebug', false, { type: 'boolean', description: nls.localize('multiSessionDebug', "True when there is more than 1 active debug session.") });
+export const CONTEXT_DISASSEMBLE_REQUEST_SUPPORTED = new RawContextKey<boolean>('disassembleRequestSupported', false, { type: 'boolean', description: nls.localize('disassembleRequestSupported', "True when the focused sessions supports disassemble request.") });
+export const CONTEXT_DISASSEMBLE_VIEW_FOCUS = new RawContextKey<boolean>('disassembleViewFocus', false, { type: 'boolean', description: nls.localize('disassembleViewFocus', "True when the Disassembly View is in focused.") });


Typo: True when the Disassembly View is in focused. -> True when the Disassembly View is focused.

weinand

I did a final review of all changes.
Most issues are minor and only affect comments or naming.

But it would be great if you could at least address the following two bigger issues:

the missing call to sendInstructionBreakpoints in sendAllBreakpointsbecause without that, the Disassembly view will show instruction breakpoints from the last debug session that are not registered with the new session (and are never hit).
check supportsInstructionBreakpoints capability before calling setInstructionBreakpoints. Currently you are checking the supportsDisassembleRequest capability (which is wrong).

After fixing these two issue we can merge the PR.

weinand · 2021-07-21T10:32:10Z

src/vs/workbench/contrib/debug/browser/debugService.ts

+			const result: DebugProtocol.DisassembledInstruction[] = [];
+
+			for (let i = 0; i < instructionCount; i++) {
+				result.push({ address: `${i + offset}`, instruction: `debugger is not available.` });


Filling the disassembly view with fake "error" instructions is fishy...
I suggest to throw an error with a correct error message, (e.g. "no active debug session" and "no disassembly available"), and catch that error at the calling site of getDisassemble() and show a proper error UI in the Disassembly view.

For the initial release this is OK, but make sure not to forget to fix this.

Removed. The disassembly view should now handle all the good and bad data from debugger.

weinand · 2021-07-21T10:37:36Z

src/vs/workbench/contrib/debug/browser/debugService.ts

@@ -978,6 +1013,26 @@ export class DebugService implements IDebugService {
 		await this.sendExceptionBreakpoints(session);
 	}

+	async getDisassemble(offset: number, instructionOffset: number, instructionCount: number): Promise<any> {


"getDisassemble" sounds wrong because "disassemble" is a verb.
I suggest "getDisassembledInstructions"

Please use correct return type Promise<DebugProtocol.DisassembledInstruction[]>

Removed function as it was unused and it is already under debugSession.

src/vs/workbench/contrib/debug/browser/debugService.ts

weinand · 2021-07-21T10:51:05Z

src/vs/workbench/contrib/debug/common/debug.ts

+
+	/**
+	 * Removes all instruction breakpoints. If address is passed only removes the instruction breakpoint with the passed address.
+	 * Notifies debug adapter of breakpoint changes.


I assume that the matching is actually based on the "string" value and not on the real "address". So "0x0F" and "15" are the same address but different strings.
Please clarify the comment.

amended comment.

weinand · 2021-07-21T10:52:48Z

src/vs/workbench/contrib/debug/common/debug.ts

@@ -940,6 +965,8 @@ export interface IDebugService {
 	 */
 	sourceIsNotAvailable(uri: uri): void;

+	getDisassemble(offset: number, instructionOffset: number, instructionCount: number): Promise<any>;


bad name "getDisassemble" and incorrect return type (see my other comment on the implementation of getDisassemble)

Removed "getDisassemble" from DebugService as it should be called from debugSession instead.

It was unused.

weinand · 2021-07-21T11:14:45Z

src/vs/workbench/contrib/debug/common/disassemblyViewInput.ts

+		return DisassemblyViewInput.ID;
+	}
+
+	static _instance: DisassemblyViewInput;


I wonder why only a single Disassembly view is supported.
In a multi-threaded debugging scenario multiplexing multiple threads into a single viewer will result in an "interesting" user experience...

But for the initial release this is OK.

ATM, it is to maintain parity with VS's disassembly view. We can have a discussion about this feature for the next iteration.

weinand · 2021-07-21T11:18:35Z

src/vs/workbench/api/browser/mainThreadDebugService.ts

@@ -337,7 +337,7 @@ export class MainThreadDebugService implements MainThreadDebugServiceShape, IDeb
 		return undefined;
 	}

-	private convertToDto(bps: (ReadonlyArray<IBreakpoint | IFunctionBreakpoint | IDataBreakpoint>)): Array<ISourceBreakpointDto | IFunctionBreakpointDto | IDataBreakpointDto> {
+	private convertToDto(bps: (ReadonlyArray<IBreakpoint | IFunctionBreakpoint | IDataBreakpoint | IInstructionBreakpoint>)): Array<ISourceBreakpointDto | IFunctionBreakpointDto | IDataBreakpointDto> {


@isidorn supporting instruction breakpoints in the debug extension API requires more work (which is not part of this PR)

Please create a feature request "Support instruction breakpoints in debug API" for this.

weinand · 2021-07-21T11:23:55Z

src/vs/workbench/contrib/debug/browser/breakpointsView.ts

+		return InstructionBreakpointsRenderer.ID;
+	}
+
+	renderTemplate(container: HTMLElement): IInstructionBreakpointTemplateData {


@isidorn we need a new icon for instruction breakpoints.

@iidorn -> @isidorn ;-)

@jogo- thanks for pointing that out

weinand · 2021-07-21T11:28:55Z

src/vs/workbench/contrib/debug/browser/debug.contribution.ts

@@ -373,6 +378,13 @@ viewsRegistry.registerViews([{ id: BREAKPOINTS_VIEW_ID, name: nls.localize('brea
 viewsRegistry.registerViews([{ id: WelcomeView.ID, name: WelcomeView.LABEL, containerIcon: icons.runViewIcon, ctorDescriptor: new SyncDescriptor(WelcomeView), order: 1, weight: 40, canToggleVisibility: true, when: CONTEXT_DEBUG_UX.isEqualTo('simple') }], viewContainer);
 viewsRegistry.registerViews([{ id: LOADED_SCRIPTS_VIEW_ID, name: nls.localize('loadedScripts', "Loaded Scripts"), containerIcon: icons.loadedScriptsViewIcon, ctorDescriptor: new SyncDescriptor(LoadedScriptsView), order: 35, weight: 5, canToggleVisibility: true, canMoveView: true, collapsed: true, when: ContextKeyExpr.and(CONTEXT_LOADED_SCRIPTS_SUPPORTED, CONTEXT_DEBUG_UX.isEqualTo('default')) }], viewContainer);

+// Register disassembly editor


better call it "view" for now:

// Register disassembly view

src/vs/workbench/contrib/debug/browser/rawDebugSession.ts

xisui-MSFT · 2021-07-21T20:33:56Z

@xisui-MSFT thanks, the scrolling behavior is much better now:

When stepping down, the line with the "current instruction" still disappears partially out of view before it is revealed.
When stepping up this does not happen but I think it would be still better if scrolling would start one line earlier so that there would be "one line of context" on both sides of the "current instruction".

@weinand Sorry there was a bug in my changes yesterday, and so the behavior is actually not what I expected. I have fixed it and the behavior now is:

if the target line is within the viewport, don't reveal
if the target line is partially within the viewport, or is out of the viewport but close to it, reveal it either at the bottom or top of the viewport, depending on stepping over or back
if the target line is far from the viewport, reveal it at the middle of the viewport.

This should avoid or mitigate most weird jumps. But then I found this behavior is different from the existing behavior in vs code. Which one do you prefer?

BTW, thanks for the mock debugger, it's very helpful!

weinand · 2021-07-21T20:44:35Z

@xisui-MSFT great to hear that Mock Debug is helpful for you too ;-)

BTW, I've added the sendInstructionBreakpoints to sendAllBreakpoints and that seems to work fine for me.

weinand · 2021-07-21T20:57:07Z

@xisui-MSFT since I'll be offline for the next 12 hours, I suggest that you merge the PR after addressing the two issues mentioned in my review. If possible please check whether the PR breaks CI.

Let's hope that we'll get a good build with the assembly view feature tomorrow morning.

weinand · 2021-07-22T08:29:06Z

@xisui-MSFT @yuehuang010 since there was no reaction on your part, I have fixed the two issues myself.

Do you still want to have this PR merged?

weinand · 2021-07-22T08:34:55Z

Today's demo:

I'm happy about this and I think we should merge.

xisui-MSFT · 2021-07-22T08:52:07Z

@weinand I think Felix is working on resolving your comments. @yuehuang010 Any updates?

weinand · 2021-07-22T09:05:16Z

Since I've fixed the only two important issues, and the other issues are minor and require no immediate code change, I think we are fine.

Should I merge?

yuehuang010 · 2021-07-22T09:20:36Z

Ship it!

weinand · 2021-07-22T09:32:31Z

Merged.
@yuehuang010 @xisui-MSFT Thanks a lot!

weinand · 2021-07-22T09:40:23Z

@yuehuang010 @xisui-MSFT I've already created a Test item which is due next Monday EOD.
I suggest that we give tester the choice to test the Disassembly view either with the C++ extension or with Mock Debug.
If you agree, then please add to the item.

I will now start to create issues for known problems and link them with the test item so that testers don't have to waste time by creating new (duplicate) issues for them...

yuehuang010 · 2021-07-22T09:42:19Z

How soon could people start using the insider?

weinand · 2021-07-22T09:50:11Z

As soon as possible or available.
Those that run VS Code out of source can use it immediately, others will have to wait until tomorrow.
But official testing will start next Tuesday.

weinand · 2021-07-22T09:53:27Z

BTW, we can still fix issues until Monday evening.
Tuesday and Wednesday is testing, and Wednesday and Thursday is fixing.
On Wednesday we can decide whether we want to ship/enable the feature in July.

vadimcn · 2021-07-24T07:14:38Z

I've tried this out in today's insiders and noticed that when scrolling down, the Disassembly View sends request with memoryReference = <address of the last instruction> and instructionOffset = 1 to fetch the next block of instructions.
My reading of the DAP spec is that the address field contains user-viewable address representation, which is not necessarily the same as what adapter uses to represent memoryReference's. IMO, the way the spec is currently written, DisassembleRequest is supposed to use the original memoryReference as the base.

yuehuang010 · 2021-07-24T08:48:02Z

Good observation. It should be a simple enough change to keep track of upper offset and lower offset.

…

On Sat, Jul 24, 2021 at 12:14 AM vadimcn ***@***.***> wrote: I've tried this out in today's insiders and noticed that when scrolling down, the Disassembly View sends request with memoryReference = <address of the last instruction> and instructionOffset = 1 to fetch the next block of instructions. My reading of the DAP spec is that the address field contains user-viewable address representation, which is not necessarily the same as what adapter uses to represent memoryReference's. IMO, the way the spec is currently written, DisassembleRequest is supposed to use the original memoryReference as the base. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#125737 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEXI5GKLDT7RLMS3QSTHGALTZJR6VANCNFSM46JTW5MA> .

weinand · 2021-08-04T09:23:38Z

Yes, @vadimcn 's observation is correct.

vscode-triage-bot assigned eamodio Jun 8, 2021

isidorn assigned isidorn and unassigned eamodio Jun 8, 2021

isidorn added the debug Debug viewlet, configurations, breakpoints, adapter issues label Jun 8, 2021

isidorn added this to the June 2021 milestone Jun 8, 2021

isidorn requested changes Jun 8, 2021

View reviewed changes

xisui-MSFT commented Jun 11, 2021

View reviewed changes

isidorn self-requested a review June 11, 2021 09:16

weinand mentioned this pull request Jun 15, 2021

Support readMemory/writeMemory from debuggers #126268

Closed

xisui-MSFT commented Jun 16, 2021

View reviewed changes

jogo- reviewed Jul 3, 2021

View reviewed changes

xisui-MSFT marked this pull request as ready for review July 7, 2021 19:25

xisui-MSFT and others added 7 commits July 7, 2021 12:35

disassembly view: initial commit

45bdf6b

Rendering table, start to work on scrolling

aada2f4

First Compile

a83ef3a

Infinite scrolling

0e7e274

Breakpoints work, but can still be improved

f4015e1

Improve breakpoints UI

01dd48e

Add instruction to viewer

fc3e9cd

weinand suggested changes Jul 21, 2021

View reviewed changes

Improve stepping UI

93c8ad6

weinand added 2 commits July 22, 2021 10:17

call sendInstructionBreakpoints for persisted breakpoints

a0e2977

check correct capability supportsInstructionBreakpoints

7fb6a2e

weinand mentioned this pull request Jul 22, 2021

Test Disassembly view #129161

Closed

2 tasks

weinand added on-testplan feature-request Request for new features or functionality labels Jul 22, 2021

Remove getdisassemble as it is not used from DebugService

c492064

weinand merged commit 4f5f5de into microsoft:main Jul 22, 2021

weinand mentioned this pull request Jul 27, 2021

Disassembly view: clicking on the active line produces a weird green color #129535

Closed

vadimcn mentioned this pull request Jul 28, 2021

Disassembly View: instruction address is not the same as a memory reference. #129655

Closed

This was referenced Aug 6, 2021

Disassembly View: Milestone 2 #129762

Open

Disassembly view: migrate to use WorkbenchList #130438

Open

github-actions bot locked and limited conversation to collaborators Sep 8, 2021


		export class DisassemblyView extends EditorPane {

		private static readonly NUM_INSTRUCTIONS_TO_LOAD = 50;


		}

		export class DisassemblyVIewContribution implements IWorkbenchContribution {

Disassembly view #125737

Disassembly view #125737

Conversation

xisui-MSFT commented Jun 8, 2021

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xisui-MSFT Jun 8, 2021 • 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

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

xisui-MSFT Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isidorn Jun 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isidorn commented Jun 8, 2021

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

isidorn commented Jun 11, 2021

xisui-MSFT Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isidorn Jun 22, 2021 • 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

Choose a reason for hiding this comment

yuehuang010 commented Jun 21, 2021

isidorn commented Jun 22, 2021

isidorn commented Jun 25, 2021

xisui-MSFT commented Jun 25, 2021

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

Choose a reason for hiding this comment

weinand left a comment • edited Loading

Choose a reason for hiding this comment

weinand Jul 21, 2021 • 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

weinand Jul 21, 2021 • 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

xisui-MSFT Jun 8, 2021 •

edited

Loading

xisui-MSFT Jun 9, 2021 •

edited

Loading

isidorn Jun 10, 2021 •

edited

Loading

xisui-MSFT Jun 16, 2021 •

edited

Loading

isidorn Jun 22, 2021 •

edited

Loading

weinand left a comment •

edited

Loading

weinand Jul 21, 2021 •

edited

Loading

weinand Jul 21, 2021 •

edited

Loading

weinand Jul 21, 2021 •

edited

Loading

weinand Jul 21, 2021 •

edited

Loading

weinand Jul 21, 2021 •

edited

Loading