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

[Pete Batard <pete@akeo.ie>]

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.

> 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.

> 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.

> 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.

Regards,

/Pete