Bugzilla – Full Text Bug Listing
| Summary: | [memory] Expression Evaluation API | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Ted Williams <ted> | ||||||||||
| Component: | Debug | Assignee: | Platform-Debug-Inbox <platform-debug-inbox> | ||||||||||
| Status: | CLOSED WONTFIX | QA Contact: | |||||||||||
| Severity: | enhancement | ||||||||||||
| Priority: | P3 | CC: | darin.eclipse, pawel.1.piech, teodor.madan | ||||||||||
| Version: | 3.5 | ||||||||||||
| Target Milestone: | --- | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| Whiteboard: | stalebug | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Ted Williams
Have you looked IWatchExpressionDelegate? It is specific to the standard debug model, but if it works for this purpose we could try to generalize it a bit. Ted, can you remind me if this is needed such that you can evaluate an expression to an address without having to create a memory block. The reason why this is needed because you don't want to create the memory block unless the expression is valid? Design:
We do not need to introduce new API to do evaluation. However, we do need to subclass from IValue to represent an address value. The idea is that client should use the ExperessionManager to create a watch expression. We can provide the proper context to do the evaluation. The watch expression delegate performs the evaluation and returns a value of type IAddressValue. IAddressValue allows us to return a BigInteger as the value which represents the address of the expression.
For M5, we would like to add the new IAddressValue API in the debug model. In addition, we would like to make the "Go To Address" bar to make use of this new API, to make sure that the API is defined properly.
API Proposal:
public Interface IAddressValue {
public BigInteger getAddressValue() throws DebugException;
}
Would IAddressValue be a subtype of IValue? (Sounds like it should be). You might want to consider asking the IValue for its BigInteger adapter rather than adding new interfaces for specific types of values. Yes, IAddressValue will be a subtype of IValue. I think BigInteger adapter approach also makes sense. However, if there are other things that we would like the address value to represent, we will not be able to do that with the BigInteger adapter. i.e. Would an address have additional properties that should be represented by the address value? e.g. address size? (In reply to comment #5) > Yes, IAddressValue will be a subtype of IValue. > > I think BigInteger adapter approach also makes sense. However, if there are > other things that we would like the address value to represent, we will not be > able to do that with the BigInteger adapter. i.e. Would an address have > additional properties that should be represented by the address value? e.g. > address size? Would it be useful to add a an IAddress interface to debug platform? This would open the question whether other memory-related interfaces should be refactored to use it. The IAddress found in CDT looks like the following. The only real additional piece of information here is getSize(). /** * Represents C/C++ address in CDT. All implementors of this interface should be * immutable, i.e. all methods should not modify objects, they should return * new object. * * Please see Addr32 and Addr64 classes to see how this interface should * be extended */ public interface IAddress extends Comparable<Object> { /** * Adds offset to address and returns new address object * which is the result * @param offset to add * @return the new address */ IAddress add(BigInteger offset); /** * Adds offset to address and returns new address object * which is the result * <br><br>Note: This method has an offset limit of Long.MAX and Long.MIN, which under some addressing schems * may impose an unnesseary limitation, see <code>IAddressa.add(BigInteger offset)</code> to handle larger offsets. * @param offset to add * @return the new address */ IAddress add(long offset); /** * Returns maximal offset possible for address. The offset * should be Identicall for all addresses of given class. * @return the max offset for this address class */ BigInteger getMaxOffset(); /** * Returns distance to address. Distance may be positive or negative * @param other address which distance is calculated to. * @return distance to address */ BigInteger distanceTo(IAddress other); /** * Returns the value of the address. */ BigInteger getValue(); /** * Returns whether this address equals the given object. * * @param obj the other object * @return <code>true</code> if the addresses are equivalent, * and <code>false</code> if they are not */ boolean equals(Object addr); /** * Return true if address is zero, i.e. minimal possible * @return true is address is zero */ boolean isZero(); /** * Return true if address is maximal, i.e. maximal possible * @return true if address is maximal */ boolean isMax(); /** * Converts address to string as an unsigned number with given radix * @param radix to use for strng conversion * @return a string representation of address */ String toString(int radix); /** * Identical to toString(10) * @return a string representation of address using a radix of 10 */ String toString(); /** * Converts address to the hex representation with '0x' prefix and * with all leading zeros. The length of returned string should be * the same for all addresses of given class. I.e. 10 for 32-bit * addresses and 18 for 64-bit addresses */ String toHexAddressString(); /** * Converts address to the binary representation with '0b' prefix and * with all leading zeros. The length of returned string should be * the same for all addresses of given class. I.e. 34 for 32-bit * addresses and 66 for 64-bit addresses */ String toBinaryAddressString(); /** * Returns amount of symbols in hex representation. Is identical to * toHexAddressString().length(). It is present for perfomance purpose. * * @return the nmber os chararcter symbols to represent this address in hex. */ int getCharsNum(); /** * Returns the address size in bytes. * * @return the number of bytes required to hold this address. */ int getSize(); } Introducing a new interface would likely ease adoption (and reduce confusion of getAdapter(...)). I would suggest to use IMemoryAddress to make it consistent with other memory view/block related interfaces. Not sure about re-using CDT's interface (should we try to keep it as small as possible?). I think Sam/Pawel will need to decide on this as they have the most experience in this area of the debug APIs. We only have one chance to get the interface correct (to avoid IMemoryAddress2...). (In reply to comment #7) > Introducing a new interface would likely ease adoption (and reduce confusion of > getAdapter(...)). I would suggest to use IMemoryAddress to make it consistent > with other memory view/block related interfaces. Not sure about re-using CDT's > interface (should we try to keep it as small as possible?). I think Sam/Pawel > will need to decide on this as they have the most experience in this area of > the debug APIs. We only have one chance to get the interface correct (to avoid > IMemoryAddress2...). > I agree to making an IMemoryAddress interace. I do not think we should reuse CDT's interface since I also want to make this interface as small as possible. Looking at the CDT's interface, here's a list of methods that I think should be included as part of this new interface: Interface IMemoryAddress { /** * Returns the value of the address. */ BigInteger getValue(); /** * Returns the address size in bytes. * * @return the number of bytes required to hold this address. */ int getSize(); } I think the rest of the methods are more for the UI to present this address or for doing calculations. I think those methods should be part of the actual IMemoryAddress implementation, rather than being part of the interface. What about address space? Should it also be modeled in the memory address? how should address space be represented? My concern with putting in an address space is that it's going to make the interface bigger, and the platform does not really support it. I think perhaps we should leave it out of the platform. (In reply to comment #8) > Looking at the CDT's interface, here's a list of methods that I think should be > included as part of this new interface: > > Interface IMemoryAddress { > /** > * Returns the value of the address. > */ > BigInteger getValue(); > > /** > * Returns the address size in bytes. > * > * @return the number of bytes required to hold this address. > */ > int getSize(); > } > > I think the rest of the methods are more for the UI to present this address or > for doing calculations. I think those methods should be part of the actual > IMemoryAddress implementation, rather than being part of the interface. +1 > What about address space? Should it also be modeled in the memory address? > how should address space be represented? My concern with putting in an address > space is that it's going to make the interface bigger, and the platform does > not really support it. I think perhaps we should leave it out of the platform. I agree. IMO each separate address space needs to be represented by a separate memory retrieval object instance, so it should be left out. OK, looks like there is agreement on the design, so +1 for someone (Sam?) to contribute for M5. Created attachment 123121 [details]
IMemoryAddress interface
It is intended that IExpression implementors optionally implement this IMemoryAddress interface. Should this be better documented in the javadoc?
(In reply to comment #11) > Created an attachment (id=123121) [details] > IMemoryAddress interface Whoa! This patch seems quite a bit different than what I thought we were converging upon. I thought we would add IMemoryAddress interface which would be accessed through IValue.getAdapter(), or returned by IMemoryValue (extending IValue) getAddress(). Created attachment 123132 [details]
updated
Created attachment 123279 [details]
Patch to IMemoryAddress
Same here :) The patch is still a bit different from what I envisioned. Here's an updated patch. Let me know if that's what you are looking for. I need to write some tests to validate this API. The platform does not have any code that exercise this API now.
(In reply to comment #14) > Same here :) The patch is still a bit different from what I envisioned. > Here's an updated patch. Let me know if that's what you are looking for. I > need to write some tests to validate this API. The platform does not have any > code that exercise this API now. It seems a bit hasty to add an interface without a concrete implementation in place. I spoke with Ted about our plans to use this interface in our product and it seems that we can keep using a workaround for now. I would also like to see an API for expression evaluation which is not so tied to the standard debug model. With more time, I could propose extensions to the IWatchExpressionDelegate... (In reply to comment #15) > It seems a bit hasty to add an interface without a concrete implementation in > place. I spoke with Ted about our plans to use this interface in our product > and it seems that we can keep using a workaround for now. I would also like to > see an API for expression evaluation which is not so tied to the standard debug > model. With more time, I could propose extensions to the > IWatchExpressionDelegate... > When discussing with Ted, we thought that this API is required for Bug 214956 (for adding the go to address bar). I agree with you that if there is no implementation, then we should not add this API. Therefore, we should only apply this patch if Bug 214956 gets in, if the API is used there. I also think that by using watch expression, you can do evaluations without having this API if the evaluations are done in your model. The reason why it's needed in the platform is that the go to address bar needs to evaluate an expression to an address, and we need a way to get to the address value. I am not sure why I understand exetending IWatchExpressionDelegate will work. (or if there is a need to extend the delegate) The delegate is for doing evaluations, but there is still no way for the UI to get to the value, if IValue does not return a BigInteger or an IMemoryAddress type. Can you elaborate? Created attachment 123406 [details] Patch with IMemoryAddress as discussed above. (In reply to comment #16) > I am not sure why I understand exetending IWatchExpressionDelegate will work. > (or if there is a need to extend the delegate) The delegate is for doing > evaluations, but there is still no way for the UI to get to the value, if > IValue does not return a BigInteger or an IMemoryAddress type. Can you > elaborate? > I'm sorry I wasn't very clear in my previous comment. From talking to Ted, it seemed that the go to address bar would be contributed by specific models as a customized rendering, and as far as I understand it this address bar has not been implemented yet. So that's what I mean that there is no client yet to prove the interface. As far as making IWatchExpressionDelegate, I'm merely referring to the fact that createWatchExpression() takes an IDebugElement argument, and IWatchExpressionResult returns an IValue, both of which link into IDebugTarget, ILaunch, etc. This would make it difficult for us to implement these interfaces using DSF, and of course we would like to use the new memory go to address bar capabilities with DSF as well :-) I think the problem could be easily addressed by adding an IWatchExpressionDelegateExtension, which uses a context of type Object, and IWatchExpressionResultExtension, which returns an IAdaptable value. And ideally adding these extension would happen along side with the address value one. That said, I if we just look at the memory address extension from this bug, I would expect to see this patch (as proposed in comment #8). The IMemoryAddress could be retrieved from IValue through getAdapter(), thus keeping the new interface compatible with other models, and with any future extension to IWatchExpressionDelegate. Finally, Ted also mentioned to me that he currently uses a workaround to obtain an address from an expression: by creating a new memory block with an expression, and then retrieving an address from it. That's why it seemed to me that the urgency for this change is not as critical. Still, if you think that adding this IMemoryAddress would be sufficient, and Ted agrees, that I say go for it :-D (In reply to comment #9) > (In reply to comment #8) > > What about address space? Should it also be modeled in the memory address? > > how should address space be represented? My concern with putting in an address > > space is that it's going to make the interface bigger, and the platform does > > not really support it. I think perhaps we should leave it out of the platform. > > I agree. IMO each separate address space needs to be represented by a separate > memory retrieval object instance, so it should be left out. Actually I would argue that address space should be left-out of memory address model. Indeed the platform could leave address space out of the model, and CDT could extend the model by adding address space representation. An expression can be evaluated to a memory addresses from different address spaces. e.g. expression "main" would evaluate usually to virtual code memory space, while expression "&gVal" would usually evaluate to virtual data memory space. This will lead to inconsistent result especially for debugging targets with Harvard architecture. There're also other examples with targets that do use multiple data address spaces and only expression evaluator has the knowledge of the address space that will have to be used by a memory retrieval. This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. If the bug is still relevant, please remove the "stalebug" whiteboard tag. |