Re: [PATCH 0/2] MdeModulePkg/EbcDxe: add EBC Debugger

["Yao, Jiewen" <jiewen.yao@intel.com>]

Comment inline.

From: Pete Batard [mailto:pete@akeo.ie]
Sent: Saturday, November 12, 2016 8:46 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH 0/2] MdeModulePkg/EbcDxe: add EBC Debugger

On 2016.11.12 07:48, Yao, Jiewen wrote:
> I met some problem on extracting patch from email. So I reviewed the
> code in https://github.com/pbatard/EbcDebugger and I assume they are same.

Sorry, no, they are not the same. Please do not use that project, as it
is missing some important parts (and also has changes that shouldn't be
part of the current proposal).

It was the project where I've been experimenting, before I cleaned
things up submit this proposal, and I am planning to update it after
this patch has been integrated to reflect the cleanup, and the other
changes we are discussing.

The actual github project to use for this proposal is:
https://github.com/pbatard/edk2/tree/EBCDebugger

> However, the master contains some non-EBC-debugger related code.

Yes it does. Please disregard that earlier github project you found, and
only use https://github.com/pbatard/edk2/tree/EBCDebugger.

> The edk2-diff branch contains an old version of EBC driver code.

The edk2-diff branch is only intended to show the changes between the
old and new code, since this patch just provides the new sources under
EBCDebugger/ wholesale, which makes it hard to see what was altered
there. Also please note that the diff is only for the content under
EBCDebugger/.

> Next time, would you please post your V2 patches to a new branch, help
> me do the review efficiently?

I'll make sure to create a github branch for V2.
[Jiewen] Thank you.

> Now I only checked the diff file. Here is some suggestion for your
> consideration:
>
> 1)      EFI_DEBUGGER_CONFIGURATION_PROTOCOL
>
> Previously, we document a way to consume this protocol. In user manual:
>
> https://sourceforge.net/projects/efidevkit/files/Documents/EBC%20Debugger%20User%20Manual.pdf/download
>
> Appendix A. Configuring the EBC Debugger under EFI Shell.
>
> But I cannot find the EbcDebuggerConfig application in EDKI. It brings
> you some confusing because you think no one is consuming this protocol.

Yeah. As I indicated in my notes I dropped the debugger configuration
protocol, because I didn't see much use for it.

> 2) (..) So I post a EDK-I style APPLICATION to my personal git-hub -
> https://github.com/jyao1/EbcDebuggerApp
>
> You can take a look. It is also BDS license code.

Thanks, I will do that.

Do you think maybe we could split adding the EBC debugger without the
configuration protocol, and then work on a separate patch to add it
back? Especially, if EbcDebuggerApp needs to be ported, I think I'd
prefer to split these 2 tasks into 2 separate series of patch, even if
it means that, for a while, the EBC debugger will not match its manual
when it comes to configuration.
[Jiewen] I think it is OK. I agree with you that this can be handled in different patch. Let's focus on current one at first.

> 3.1.2) Use Library - such EbcDebuggerLib.
>
> Personally, my preference is 3.1.2) because it can make code clean and
> align with our other debugger feature.

Okay. I'll try to work with 3.1.2 then, as I don't have a strong preference.
[Jiewen] Thank you.

> 3.2) EbcExecute(), I cannot find below original code. Would you please
> double check?
>
>     //
>     // Verify the opcode is in range. Otherwise generate an exception.
>     //
>     if ((*VmPtr->Ip & OPCODE_M_OPCODE) >= (sizeof (mVmOpcodeTable) /
> sizeof (mVmOpcodeTable[0]))) {
>       EbcDebugSignalException (EXCEPT_EBC_INVALID_OPCODE,
> EXCEPTION_FLAG_FATAL, VmPtr);
>       Status = EFI_UNSUPPORTED;
>       goto Done;
>     }

This code is unrelated to the EBC Debugger and was removed in 2009 in
the official EDK2:
https://github.com/tianocore/edk2/commit/ead7e7dc748750e88a1d1d5810c4550edeabb22f

> 3.3) ExecuteBREAK(), I cannot find below original code. Would you please
> double check?
>
>   case 3:
>     VmPtr->StopFlags |= STOPFLAG_BREAKPOINT;
>     VmPtr->Ip += 2; <======== here
>     //
>     // See if someone has registered a handler
>     //
>     EbcDebugSignalException (
>       EXCEPT_EBC_BREAKPOINT,
>       EXCEPTION_FLAG_NONE,
>       VmPtr
>       );
>     return EFI_SUCCESS; <======== here
>     break;

Similarly the 2 lines you point to have never been present in the EDK2
version I see of ExecuteBREAK():
https://github.com/tianocore/edk2/blob/53c71d097b13311e2bd8dda6ae54b5766a1c7d6d/MdeModulePkg/Universal/EbcDxe/EbcExecute.c#L1115-L1125

If the sections you point to need to be modified, I think that would be
better left to a different patch, as these would apply to the generic
EbcDxe module and not the EBC Debugger.
[Jiewen] That makes sense. We can use separate patch to handle these 2 enhancement. I agree with you.


Regards,

/Pete