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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
