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

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

[Pete Batard]

Hi,

This is v2 of a patchset to add the EBC Debugger back into EDK2.

Notes regarding this updated version are as follows:

0. You can find a github repo with this series of patch at:
https://github.com/pbatard/edk2/tree/EbcDebugger_v2 (patches at 
https://github.com/pbatard/edk2/commits/EbcDebugger_v2)

1. The series was broken down in 3 parts, with:
- changes that impact the existing driver code (1/3)
- changes that introduce the debugger code (2/3)
- changes related to factorizing and cleaning up the EBC headers (3/3)

2. This version introduces EbcDebuggerHook.c which contains the null 
version of the Debugger hooks, which enables keeping EBC driver 
compilation as it was, and avoids the use of a macro or Pcd for the 
debugger compilation. Note that I preferred to go with 
'EbcDebuggerHook.c' rather than 'EbcDebuggerHookNull.c', as I think it 
fits better with 'EbcDebuggerHook.h' and the comments in the header make 
it very explicit that this only contains the null version of the hooks.

3. Because a version is used by both the debugger and the driver, the 
prototype for EbcDebugSignalException() was also moved from EbcInt.h to 
EbcDebuggerHook.h

4. I chose to move the definitions that relate to the EBC VM into 
EbcVmTest.h and the rest (opcode related) to Ebc.h, as I think it made 
more sense.
On a semi related note, I think that, at some stage, we should rename 
(or split) EbcVmTest.h because it is confusing as it's not used for 
testing only (all the EBC VM implementations use that file for the 
private EBC VM structure).  But of course, that is something that is 
better done outside of this patch series.

Regards,

/Pete

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

[Laszlo Ersek]

Pete,

On 11/14/16 12:06, Pete Batard wrote:
> Hi,
> 
> This is v2 of a patchset to add the EBC Debugger back into EDK2.

can you please post multi-patch series with:

  git send-email --thread --no-chain-reply-to

(I'm unsure how this can be set on Windows git, but I believe other
Windows users could advise you on that.)

Without proper threading, patches that belong together scatter even in
MUAs that support threaded views.

You don't seem to have received comments for v2 just yet, so I propose
to repost ASAP with threading enabled (with subject prefix "PATCH v2
RESEND"), so that once the comments start flowing in, we may have a
nicely rooted discussion.

Thanks!
Laszlo

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

[Laszlo Ersek]

On 11/14/16 12:51, Laszlo Ersek wrote:

>   git send-email --thread --no-chain-reply-to

(I apologize for tooting my own horn, but for completeness: this is
mentioned in
<https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-05>:

 git config sendemail.chainreplyto false
 git config sendemail.thread       true

)

Thanks
Laszlo

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

[Pete Batard]

Hi Laszlo,

Thanks for the pointers... which I probably should have read sooner - 
sorry about that!

It's certainly a wild ride to get git on Windows to e-mail formatted 
patches to the list, but hopefully, the new set has been posted and will 
be more palatable.

Regards,

/Pete

On 2016.11.14 11:53, Laszlo Ersek wrote:
> On 11/14/16 12:51, Laszlo Ersek wrote:
>
>>   git send-email --thread --no-chain-reply-to
>
> (I apologize for tooting my own horn, but for completeness: this is
> mentioned in
> <https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-05>:
>
>  git config sendemail.chainreplyto false
>  git config sendemail.thread       true
>
> )
>
> Thanks
> Laszlo
>

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

[Yao, Jiewen]

Thanks Pete.
Now I can apply this version GIT patch.
Good update, and thanks Laszlo's BKM on GIT configuration.

I like your 3-step patch. Very clear.

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.

Thank you
Yao Jiewen

From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Pete Batard
Sent: Monday, November 14, 2016 9:18 PM
To: 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

Hi Laszlo,

Thanks for the pointers... which I probably should have read sooner -
sorry about that!

It's certainly a wild ride to get git on Windows to e-mail formatted
patches to the list, but hopefully, the new set has been posted and will
be more palatable.

Regards,

/Pete

On 2016.11.14 11:53, Laszlo Ersek wrote:
> On 11/14/16 12:51, Laszlo Ersek wrote:
>
>>   git send-email --thread --no-chain-reply-to
>
> (I apologize for tooting my own horn, but for completeness: this is
> mentioned in
> <https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-05>:
>
>  git config sendemail.chainreplyto false
>  git config sendemail.thread       true
>
> )
>
> Thanks
> Laszlo
>

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

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

[Pete Batard]

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

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

[Yao, Jiewen]

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

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

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

[Yao, Jiewen]

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