From mboxrd@z Thu Jan  1 00:00:00 1970
Authentication-Results: mx.groups.io;
 dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.136, mailfrom: jordan.l.justen@intel.com)
Received: from mga12.intel.com (mga12.intel.com [192.55.52.136])
 by groups.io with SMTP; Fri, 16 Aug 2019 11:29:08 -0700
X-Amp-Result: SKIPPED(no attachment in message)
X-Amp-File-Uploaded: False
Received: from fmsmga007.fm.intel.com ([10.253.24.52])
  by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Aug 2019 11:29:08 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.64,394,1559545200"; 
   d="scan'208";a="178834945"
Received: from adnanrez-mobl.amr.corp.intel.com (HELO localhost) ([10.255.228.102])
  by fmsmga007.fm.intel.com with ESMTP; 16 Aug 2019 11:29:07 -0700
MIME-Version: 1.0
In-Reply-To: <20190816021437.7516-10-michael.d.kinney@intel.com>
References: <20190816021437.7516-1-michael.d.kinney@intel.com> <20190816021437.7516-10-michael.d.kinney@intel.com>
Subject: Re: [Patch V4 09/10] EmulatorPkg/Sec: Change local variable scope for XCODE5
To: Michael D Kinney <michael.d.kinney@intel.com>, devel@edk2.groups.io
From: "Jordan Justen" <jordan.l.justen@intel.com>
Cc: Ray Ni <ray.ni@intel.com>, Andrew Fish <afish@apple.com>
Message-ID: <156598014713.11297.11793301157846922949@jljusten-skl>
User-Agent: alot/0.8
Date: Fri, 16 Aug 2019 11:29:07 -0700
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

On 2019-08-15 19:14:36, Michael D Kinney wrote:
> The local variable PpiArray[10] is declared in middle
> of the SEC module _ModuleEntryPoint().  When building
> for XCODE5 with optimizations enabled, this results in
> corruption and an exception.

It looks like with old code, the scope containing PpiArray was closed,
but a dangling reference to had been made to it's location on the
stack. So, I think the change is good but we should add this extra
detail to the commit message.

-Jordan

> The fix is to move the
> declaration of PpiArray[10] to the standard location
> at the beginning of the function so the storage for
> this local variable is allocated for the entire
> lifetime of the function.
>=20
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Andrew Fish <afish@apple.com>
> ---
>  EmulatorPkg/Sec/Sec.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
>=20
> diff --git a/EmulatorPkg/Sec/Sec.c b/EmulatorPkg/Sec/Sec.c
> index 701032233b..b734d2bb87 100644
> --- a/EmulatorPkg/Sec/Sec.c
> +++ b/EmulatorPkg/Sec/Sec.c
> @@ -75,6 +75,7 @@ _ModuleEntryPoint (
>    EFI_PEI_PPI_DESCRIPTOR    *SecPpiList;
>    UINTN                     SecReseveredMemorySize;
>    UINTN                     Index;
> +  EFI_PEI_PPI_DESCRIPTOR    PpiArray[10];
> =20
>    EMU_MAGIC_PAGE()->PpiList =3D PpiList;
>    ProcessLibraryConstructorList ();
> @@ -104,16 +105,13 @@ _ModuleEntryPoint (
>    SecCoreData->PeiTemporaryRamBase =3D (VOID *)((UINTN)SecCoreData->PeiT=
emporaryRamBase + SecReseveredMemorySize);
>    SecCoreData->PeiTemporaryRamSize -=3D SecReseveredMemorySize;
>  #else
> -  {
> -    //
> -    // When I subtrack from SecCoreData->PeiTemporaryRamBase PEI Core cr=
ashes? Either there is a bug
> -    // or I don't understand temp RAM correctly?
> -    //
> -    EFI_PEI_PPI_DESCRIPTOR    PpiArray[10];
> +  //
> +  // When I subtrack from SecCoreData->PeiTemporaryRamBase PEI Core cras=
hes? Either there is a bug
> +  // or I don't understand temp RAM correctly?
> +  //
> =20
> -    SecPpiList =3D &PpiArray[0];
> -    ASSERT (sizeof (PpiArray) >=3D SecReseveredMemorySize);
> -  }
> +  SecPpiList =3D &PpiArray[0];
> +  ASSERT (sizeof (PpiArray) >=3D SecReseveredMemorySize);
>  #endif
>    // Copy existing list, and append our entries.
>    CopyMem (SecPpiList, PpiList, sizeof (EFI_PEI_PPI_DESCRIPTOR) * Index);
> --=20
> 2.21.0.windows.1
>=20