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

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

Hi Pete
Maybe you can consider preparing V3, since there is no much other feedback.


I forget to mention that: One process we need to do is to run BaseTools\Scripts\PatchCheck.py to check if a patch meets check in criteria.

You can find info at https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format
and https://github.com/tianocore/tianocore.github.io/wiki/Code-Style-C

I found PatchCheck.py complain a lot for these 3 patches, especially [edk2]-[PATCH-(RESEND)-v2-2-3]-MdeModulePkg-EbcDxe-add-EBC-Debugger.patch. :)

Please run the tool and make sure zero issue reported.
If you have any question, feel free to ask please.

Thank you
Yao Jiewen

From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao, Jiewen
Sent: Monday, November 14, 2016 10:39 PM
To: Pete Batard <pete@akeo.ie>; Laszlo Ersek <lersek@redhat.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@lists.01.org <edk2-devel@ml01.01.org>
Subject: Re: [edk2] [PATCH v2 0/3] MdeModulePkg/EbcDxe: add EBC Debugger

1) Maybe we can wait for a while to see if there is commend from other people. :)

2) I do not we need move all to EbcVmTest.h.
As long as the OPCODE/OPRANG are defined in UEFI spec. It is OK to keep it in MdePkg.

We have similar examples, such as ACPI related OPCODE in MdePkg\Include\IndustryStandard\AcpiAml.h
And all device path related definition in MdePkg\Include\Protocol\DevicePath.h

Thank you
Yao Jiewen

From: Pete Batard [mailto:pete@akeo.ie]
Sent: Monday, November 14, 2016 10:11 PM
To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Cc: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> <edk2-devel@ml01.01.org<mailto:edk2-devel@ml01.01.org>>
Subject: Re: [edk2] [PATCH v2 0/3] MdeModulePkg/EbcDxe: add EBC Debugger

On 2016.11.14 13:58, Yao, Jiewen wrote:
> One minor concern I have is that: I found you include below in
> Protocol/Ebc.h
>
> #define GETOPERANDS(pVM)  (UINT8) (*(UINT8 *) (pVM->Ip + 1))
> #define GETOPCODE(pVM)    (UINT8) (*(UINT8 *) pVM->Ip)
> #define OPERAND1_REGDATA(pvm, op) pvm->Gpr[OPERAND1_REGNUM (op)]
> #define OPERAND2_REGDATA(pvm, op) pvm->Gpr[OPERAND2_REGNUM (op)]
>
> However, pvm->Gpr and pVM->Ip are defined in Protocol/EbcVmTest.h.
>
> I recommend that we had better move PVM related definition there.

I agree. Do you want me to re-send a patch for that?

On that subject, I've also been wondering on whether we might just move
all the operand related definitions into EbcVmTest.h and leave Ebc.h as
it was.

The more I think about it, the more I wonder if a header that's just
meant to define the EBC interpreter interface is the best place to have
EBC internal definitions. Maybe the header that is intended for use for
the actual implementation of the EBC VM (EbcVmTest.h) is where we should
move everything into?

Regards,

/Pete
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel