Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 328168

Summary: Disassembly view does not align address enter new address manually
Product: [Tools] CDT Reporter: Patrick Chuong <pchuong>
Component: cdt-debug-dsfAssignee: Patrick Chuong <pchuong>
Status: RESOLVED FIXED QA Contact: Pawel Piech <pawel.1.piech>
Severity: normal    
Priority: P3 CC: aleherb+eclipse, cdtdoug
Version: 7.0Flags: pchuong: iplog-
Target Milestone: 8.0   
Hardware: PC   
OS: Windows XP   
Whiteboard:
Attachments:
Description Flags
Address alignment patch
none
Address alignment patch (reworked) pchuong: iplog-

Description Patrick Chuong CLA 2010-10-19 14:37:54 EDT 
Build Identifier: 7.0.0.201006141710

The addresses of the disassembly does not align to a valid code address alignment. For example, if "main" is at address 0xff80 and the previous opCode address is at 0xff7c, than entering 0xff7e (or "main-1") should be aligned to 0xff7c.

Reproducible: Always
Comment 1 Anton Leherbauer CLA 2010-10-22 05:04:15 EDT 
I don't see how the disassembly view can determine what the proper start address of an instruction is.  It's up to the service to provide this alignment.

Related to that, I noticed that due to unaligned requests for disassembly valid instructions can be overridden when the service does not provide this alignment.  I think we need to mark instruction on key points (frame addresses) and don't override those.
Comment 2 Patrick Chuong CLA 2010-10-22 09:50:36 EDT 
One one to solve the initial aligment issue is: The disassembly view calls to the service to perform the alignment after the expression evaluation, than pass the aligned address to the disassembly service API to request disassembly instructions. 

Can the disassembly service API returns instructions from the request API with the aligned address? i.e if the API request 0x20, can service returns 0x1f for the instruction? If it works this way, than we need a way to know whether the getInstructions is an initial request or not. For the initial request, address alignment is required. If not, than the service doesn't need to alignment the address, it can assume the address is aligned. Alignment can be pretty expensive, it is best to avoid alignment operation as much as possible.

In our version of the disassembly view (TI-disassembly view), we tag each instruction if it align, so that un-aligned instruction will not overriden the aligned instruction. I think this is the same as you have mentioned. Also, the un-aligned instruction line has a different font style to indicate to the user the instruction might unalign (incorrect assembly instruction).
Comment 3 Anton Leherbauer CLA 2010-10-28 03:18:15 EDT 
(In reply to comment #2)
> One one to solve the initial aligment issue is: The disassembly view calls to
> the service to perform the alignment after the expression evaluation, than pass
> the aligned address to the disassembly service API to request disassembly
> instructions. 
> 
> Can the disassembly service API returns instructions from the request API with
> the aligned address? i.e if the API request 0x20, can service returns 0x1f for
> the instruction? If it works this way, than we need a way to know whether the
> getInstructions is an initial request or not. For the initial request, address
> alignment is required. If not, than the service doesn't need to alignment the
> address, it can assume the address is aligned. Alignment can be pretty
> expensive, it is best to avoid alignment operation as much as possible.

I would assume from the service implementation to always check for the proper alignment of any given start address (if it has this capability at all).
If performance is a problem, the service itself could implement some form of caching about well-known address alignments.
I don't think the view is a good place to cache this information, although it could provide some form of hint whether a start address is considered aligned.
E.g. the current PC address can always be considered properly aligned.

> In our version of the disassembly view (TI-disassembly view), we tag each
> instruction if it align, so that un-aligned instruction will not overriden the
> aligned instruction. I think this is the same as you have mentioned. Also, the
> un-aligned instruction line has a different font style to indicate to the user
> the instruction might unalign (incorrect assembly instruction).

Yes, I think this makes sense especially if the service does not have the capability to correct the alignment of a start address.
Comment 4 Patrick Chuong CLA 2010-10-28 09:43:25 EDT 
Regarding user enter start address manually, I am not proposing to have the view to cache any info. I was proposing to have a new API in the disassembly service to query the service to align an address when it is enter manually. So, everytime an address is enter manually, the view will first call to the service align the address, than pass the aligned address to the getInstruction() API. This will not require any caching in the view or the service.

Also, if the address alignment query failed, than the view can indicate the instruction lines differently than the aligned instruction lines.
Comment 5 Anton Leherbauer CLA 2010-10-29 03:35:54 EDT 
(In reply to comment #4)
> Regarding user enter start address manually, I am not proposing to have the
> view to cache any info. I was proposing to have a new API in the disassembly
> service to query the service to align an address when it is enter manually. So,
> everytime an address is enter manually, the view will first call to the service
> align the address, than pass the aligned address to the getInstruction() API.
> This will not require any caching in the view or the service.
> 
> Also, if the address alignment query failed, than the view can indicate the
> instruction lines differently than the aligned instruction lines.

Ok, maybe I misunderstood what you intend to do.
If I got this right, you suggest to add an API to align an address to the next valid instruction and any start address given to get*Instructions() can be considered already aligned by the service implementation.
Sounds reasonable, do you intend to post a patch for that?
Comment 6 Patrick Chuong CLA 2010-10-29 09:37:37 EDT 
I'll work on a patch, will submit it when it is ready. Thanks.
Comment 7 Patrick Chuong CLA 2010-11-03 15:16:51 EDT 
Created attachment 182323 [details]
Address alignment patch

This patch will align the starting address when the user enters the text in the combo box in the toolbar and as well as from the "Go to Address" dialog box.
Comment 8 Patrick Chuong CLA 2010-12-17 18:03:48 EST 
Hi Anton, did you get a chance to review this patch? I would like to commit this to head if it is OK with you.
Comment 9 Anton Leherbauer CLA 2010-12-20 04:25:45 EST 
(In reply to comment #8)
> Hi Anton, did you get a chance to review this patch? I would like to commit
> this to head if it is OK with you.

I'd rather apply the alignment to the start address for which disassembly is requested.  That's the only point where an aligned address is actually required.

Other issues are:
- wrong @since tag (@since 8.0)
- wording: alignedOpCodeAddress -> alignOpCodeAddress
- alignOpcodeAddress should not be applied by evaluateExpression()
  (this method is for the hover only)
- The synchronous method alignOpcodeAddress defined by IDisassemblyBackend2 is 
  obsolete then.
Comment 10 Patrick Chuong CLA 2010-12-20 15:49:08 EST 
Created attachment 185594 [details]
Address alignment patch (reworked)

Anton, I have updated the patch against HEAD, and make the changes that you have suggested. Please take a look when you get a chance and see if I have done it correct or not. thanks.
Comment 11 Anton Leherbauer CLA 2010-12-21 05:16:39 EST 
Looks good to me.
Comment 12 Patrick Chuong CLA 2010-12-21 11:56:27 EST 
Committed patch to HEAD.