On 09/26/18 04:18, Wang, Jian J wrote: > Hi, > > Since the patch will introduce "#if defined(...)" macro in code, which > violates > edk2 coding style, it's suggested to add exception to static checker. > > I'll wait for one or two days in case there's other suggestions. If no > objection > then, I'll withdraw this patch and close BZ#1186 as not-fix.
If we can *selectively* suppress this one warning from the static checker (saying that "yeah we know what we are doing"), then I agree WONTFIX is acceptable for the BZ. It's not optimal IMO (I think there would be value in a generic facility for getting the stack pointer -- several places in edk2 want to know the stack pointer), but it is acceptable. (As far as I'm concerned anyway.) Thanks Laszlo >> -----Original Message----- >> From: Laszlo Ersek [mailto:ler...@redhat.com] >> Sent: Wednesday, September 19, 2018 7:17 PM >> To: Justen, Jordan L <jordan.l.jus...@intel.com>; Wang, Jian J >> <jian.j.w...@intel.com>; edk2-devel@lists.01.org >> Cc: Wu, Hao A <hao.a...@intel.com>; Bi, Dandan <dandan...@intel.com>; >> Dong, Eric <eric.d...@intel.com>; Kinney, Michael D >> <michael.d.kin...@intel.com> >> Subject: Re: [edk2] [PATCH] UefiCpuPkg/CpuMpPei: fix unsafe way to get stack >> pointer >> >> On 09/18/18 20:02, Jordan Justen wrote: >>> I guess the git config sendemail.from setting did not help your >>> patches. ?? It still is coming through with a From field of >>> <edk2-devel-boun...@lists.01.org>. >>> >>> Regarding this patch, I suppose it is worth asking if &StackBase in >>> the old code could possibly be an address not on the stack. I don't >>> think it is possible, and I'm guessing the C specification would >>> probably back that up. >>> >>> It can be unsafe to get an address of something on the stack and then >>> refer to that address after the variable is no longer in scope. I >>> suspect this is what the static checker is noticing. By calling >>> SetJump, aren't we just doing the same thing, but hiding what we are >>> doing from the static checker? >> >> Yep, we're totally doing that. >> >> Thanks, >> Laszlo >> >>> >>> So, can't we just tell the static checker to ignore the error because >>> we know what we are doing? >>> >>> -Jordan >>> >>> On 2018-09-18 02:04:48, wrote: >>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1186 >>>> >>>> This patch uses SetJump() to get the stack pointer from esp/rsp >>>> register to replace local variable way, which was marked by static >>>> code checker as an unsafe way. >>>> >>>> Cc: Dandan Bi <dandan...@intel.com> >>>> Cc: Hao A Wu <hao.a...@intel.com> >>>> Cc: Eric Dong <eric.d...@intel.com> >>>> Cc: Laszlo Ersek <ler...@redhat.com> >>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>> Signed-off-by: Jian J Wang <jian.j.w...@intel.com> >>>> --- >>>> UefiCpuPkg/CpuMpPei/CpuMpPei.h | 8 ++++++++ >>>> UefiCpuPkg/CpuMpPei/CpuPaging.c | 9 +++++++-- >>>> 2 files changed, 15 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.h >> b/UefiCpuPkg/CpuMpPei/CpuMpPei.h >>>> index d097a66aa8..fe61f5e3bc 100644 >>>> --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.h >>>> +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.h >>>> @@ -35,6 +35,14 @@ >>>> >>>> extern EFI_PEI_PPI_DESCRIPTOR mPeiCpuMpPpiDesc; >>>> >>>> +#if defined (MDE_CPU_IA32) >>>> +#define CPU_STACK_POINTER(Context) ((Context).Esp) >>>> +#elif defined (MDE_CPU_X64) >>>> +#define CPU_STACK_POINTER(Context) ((Context).Rsp) >>>> +#else >>>> +#error CPU type not supported! >>>> +#endif >>>> + >>>> /** >>>> This service retrieves the number of logical processor in the platform >>>> and the number of those logical processors that are enabled on this >>>> boot. >>>> diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c >> b/UefiCpuPkg/CpuMpPei/CpuPaging.c >>>> index c7e0822452..997c20c26e 100644 >>>> --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c >>>> +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c >>>> @@ -517,9 +517,14 @@ GetStackBase ( >>>> IN OUT VOID *Buffer >>>> ) >>>> { >>>> - EFI_PHYSICAL_ADDRESS StackBase; >>>> + EFI_PHYSICAL_ADDRESS StackBase; >>>> + BASE_LIBRARY_JUMP_BUFFER Context; >>>> >>>> - StackBase = (EFI_PHYSICAL_ADDRESS)(UINTN)&StackBase; >>>> + // >>>> + // Retrieve stack pointer from current processor context. >>>> + // >>>> + SetJump (&Context); >>>> + StackBase = (EFI_PHYSICAL_ADDRESS)CPU_STACK_POINTER (Context); >>>> StackBase += BASE_4KB; >>>> StackBase &= ~((EFI_PHYSICAL_ADDRESS)BASE_4KB - 1); >>>> StackBase -= PcdGet32(PcdCpuApStackSize); >>>> -- >>>> 2.16.2.windows.1 >>>> >>>> _______________________________________________ >>>> edk2-devel mailing list >>>> edk2-devel@lists.01.org >>>> https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
