[edk2-devel] [PATCH 0/1] MdePkg/BaseLib: Fix undefined symbol when compiling with Visual Studio

[edk2-devel] [PATCH 0/1] MdePkg/BaseLib: Fix undefined symbol when compiling with Visual Studio

[Pete Batard via groups.io]

The following patch fixes a regression/breakage that currently prevents
compilation of MdePkg for AARCH64 with Visual Studio 2022. This regression
was introduced with the patch that was discussed in October 2020 at:
https://edk2.groups.io/g/devel/topic/77247140#msg65813 and that was
eventually integrated in September 2023.

The full error can be seen on a real life example at:
https://github.com/pbatard/EfiFs/actions/runs/8988513468/job/24689531001#step:8:220

Because this is a regression/breakage for one of the major toolchains, and
the fix is expected to be low impact, I would appreciate if this could be
speed-tracked for review and integration for the 202405 edk2 release, which
is currently in soft freeze.

Also, since this appears not to be the case, and it would have helped with
this issue being caught during patch integration rather than 6 months down
the line, I would also very much like to push for a VS2019/AARCH64 CI
pipeline to be added to the edk2 patch validation process.

Thanks,

/Pete

Pete Batard (1):
  MdePkg/BaseLib: Fix undefined symbol when compiling with Visual Studio

 MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm | 1 +
 1 file changed, 1 insertion(+)

--
2.45.0.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118665): https://edk2.groups.io/g/devel/message/118665
Mute This Topic: https://groups.io/mt/105977469/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 1/1] MdePkg/BaseLib: Fix undefined symbol when compiling with Visual Studio

[Pete Batard via groups.io]

Commit 80bbea192aa44ab664ba8be29ac06c83f246e99c introduced a regression
resulting in 'error A2023: undefined symbol: InternalAssertJumpBuffer'
when compling MdePkg for AARCH64 with Visual Studio.
Fix this by adding the relevant EXTERN reference.

Signed-off-by: Pete Batard <pete@akeo.ie>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
---
 MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
index 6ec8f35f2c9f..fa161e25f517 100644
--- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
+++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
@@ -7,6 +7,7 @@
 
   EXPORT SetJump
   EXPORT InternalLongJump
+  EXTERN InternalAssertJumpBuffer
   AREA BaseLib_LowLevel, CODE, READONLY
 
 #define GPR_LAYOUT                          \
-- 
2.45.0.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118666): https://edk2.groups.io/g/devel/message/118666
Mute This Topic: https://groups.io/mt/105977470/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] MdePkg/BaseLib: Fix undefined symbol when compiling with Visual Studio

[Ard Biesheuvel]

On Wed, 8 May 2024 at 10:52, Pete Batard <pete@akeo.ie> wrote:
>
> Commit 80bbea192aa44ab664ba8be29ac06c83f246e99c introduced a regression
> resulting in 'error A2023: undefined symbol: InternalAssertJumpBuffer'
> when compling MdePkg for AARCH64 with Visual Studio.
> Fix this by adding the relevant EXTERN reference.
>
> Signed-off-by: Pete Batard <pete@akeo.ie>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

Note that the same issue has been raised two months ago, and a similar
fix proposed

https://openfw.io/edk2-devel/20240320025130.599086-1-adam.liu@tw.synaptics.com/

so IMHO this qualifies for inclusion in the stable tag.


> ---
>  MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
> index 6ec8f35f2c9f..fa161e25f517 100644
> --- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
> +++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
> @@ -7,6 +7,7 @@
>
>    EXPORT SetJump
>    EXPORT InternalLongJump
> +  EXTERN InternalAssertJumpBuffer
>    AREA BaseLib_LowLevel, CODE, READONLY
>
>  #define GPR_LAYOUT                          \
> --
> 2.45.0.windows.1
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118667): https://edk2.groups.io/g/devel/message/118667
Mute This Topic: https://groups.io/mt/105977470/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] MdePkg/BaseLib: Fix undefined symbol when compiling with Visual Studio

[Ard Biesheuvel]

On Wed, 8 May 2024 at 11:07, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 8 May 2024 at 10:52, Pete Batard <pete@akeo.ie> wrote:
> >
> > Commit 80bbea192aa44ab664ba8be29ac06c83f246e99c introduced a regression
> > resulting in 'error A2023: undefined symbol: InternalAssertJumpBuffer'
> > when compling MdePkg for AARCH64 with Visual Studio.
> > Fix this by adding the relevant EXTERN reference.
> >
> > Signed-off-by: Pete Batard <pete@akeo.ie>
> > Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>
> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
>
> Note that the same issue has been raised two months ago, and a similar
> fix proposed
>
> https://openfw.io/edk2-devel/20240320025130.599086-1-adam.liu@tw.synaptics.com/
>
> so IMHO this qualifies for inclusion in the stable tag.
>

BTW the existence of this issue appears to imply that the VS RELEASE
build does not #define MDEPKG_NDEBUG. Is that an oversight?

>
> > ---
> >  MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
> > index 6ec8f35f2c9f..fa161e25f517 100644
> > --- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
> > +++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
> > @@ -7,6 +7,7 @@
> >
> >    EXPORT SetJump
> >    EXPORT InternalLongJump
> > +  EXTERN InternalAssertJumpBuffer
> >    AREA BaseLib_LowLevel, CODE, READONLY
> >
> >  #define GPR_LAYOUT                          \
> > --
> > 2.45.0.windows.1
> >


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118668): https://edk2.groups.io/g/devel/message/118668
Mute This Topic: https://groups.io/mt/105977470/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] MdePkg/BaseLib: Fix undefined symbol when compiling with Visual Studio

[Pete Batard via groups.io]

Hi Ard,

Thanks for the quick review.

Note that as opposed to the previous one you referenced, that patches 
both the .S and the .asm, my submission only patches the .asm, so it's 
probably better to use Adam Liu's for integration (who was the first to 
propose a fix anyway).

As to your other question, see inline:

On 2024.05.08 10:08, Ard Biesheuvel wrote:
> On Wed, 8 May 2024 at 11:07, Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> On Wed, 8 May 2024 at 10:52, Pete Batard <pete@akeo.ie> wrote:
>>>
>>> Commit 80bbea192aa44ab664ba8be29ac06c83f246e99c introduced a regression
>>> resulting in 'error A2023: undefined symbol: InternalAssertJumpBuffer'
>>> when compling MdePkg for AARCH64 with Visual Studio.
>>> Fix this by adding the relevant EXTERN reference.
>>>
>>> Signed-off-by: Pete Batard <pete@akeo.ie>
>>> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
>>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>>
>> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
>>
>> Note that the same issue has been raised two months ago, and a similar
>> fix proposed
>>
>> https://openfw.io/edk2-devel/20240320025130.599086-1-adam.liu@tw.synaptics.com/
>>
>> so IMHO this qualifies for inclusion in the stable tag.
>>
> 
> BTW the existence of this issue appears to imply that the VS RELEASE
> build does not #define MDEPKG_NDEBUG. Is that an oversight?

In my testing with VS2022 (with '-b RELEASE'), adding:

#ifdef MDEPKG_NDEBUG
#error MDEPKG_NDEBUG is defined
#endif

to SetJump.c does produce the expected:

d:\edk2\MdePkg\Library\BaseLib\SetJump.c(12): fatal error C1189: #error: 
  MDEPKG_NDEBUG is defined

So as far as I can tell, MDEPKG_NDEBUG is properly defined.

Regards,

/Pete

> 
>>
>>> ---
>>>   MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
>>> index 6ec8f35f2c9f..fa161e25f517 100644
>>> --- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
>>> +++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
>>> @@ -7,6 +7,7 @@
>>>
>>>     EXPORT SetJump
>>>     EXPORT InternalLongJump
>>> +  EXTERN InternalAssertJumpBuffer
>>>     AREA BaseLib_LowLevel, CODE, READONLY
>>>
>>>   #define GPR_LAYOUT                          \
>>> --
>>> 2.45.0.windows.1
>>>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118669): https://edk2.groups.io/g/devel/message/118669
Mute This Topic: https://groups.io/mt/105977470/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] MdePkg/BaseLib: Fix undefined symbol when compiling with Visual Studio

[Ard Biesheuvel]

On Wed, 8 May 2024 at 11:21, Pete Batard <pete@akeo.ie> wrote:
>
> Hi Ard,
>
> Thanks for the quick review.
>
> Note that as opposed to the previous one you referenced, that patches
> both the .S and the .asm, my submission only patches the .asm, so it's
> probably better to use Adam Liu's for integration (who was the first to
> propose a fix anyway).
>

Yeah, we'll end up merging the other patch, most likely. But thanks
for reminding us of this issue - it does need fixing.

> As to your other question, see inline:
>
> On 2024.05.08 10:08, Ard Biesheuvel wrote:
> > On Wed, 8 May 2024 at 11:07, Ard Biesheuvel <ardb@kernel.org> wrote:
> >>
> >> On Wed, 8 May 2024 at 10:52, Pete Batard <pete@akeo.ie> wrote:
> >>>
> >>> Commit 80bbea192aa44ab664ba8be29ac06c83f246e99c introduced a regression
> >>> resulting in 'error A2023: undefined symbol: InternalAssertJumpBuffer'
> >>> when compling MdePkg for AARCH64 with Visual Studio.
> >>> Fix this by adding the relevant EXTERN reference.
> >>>
> >>> Signed-off-by: Pete Batard <pete@akeo.ie>
> >>> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> >>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> >>
> >> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> >>
> >> Note that the same issue has been raised two months ago, and a similar
> >> fix proposed
> >>
> >> https://openfw.io/edk2-devel/20240320025130.599086-1-adam.liu@tw.synaptics.com/
> >>
> >> so IMHO this qualifies for inclusion in the stable tag.
> >>
> >
> > BTW the existence of this issue appears to imply that the VS RELEASE
> > build does not #define MDEPKG_NDEBUG. Is that an oversight?
>
> In my testing with VS2022 (with '-b RELEASE'), adding:
>
> #ifdef MDEPKG_NDEBUG
> #error MDEPKG_NDEBUG is defined
> #endif
>
> to SetJump.c does produce the expected:
>
> d:\edk2\MdePkg\Library\BaseLib\SetJump.c(12): fatal error C1189: #error:
>   MDEPKG_NDEBUG is defined
>
> So as far as I can tell, MDEPKG_NDEBUG is properly defined.
>

The reference to InternalAssertJumpBuffer was intended to only be
emitted if MDEPKG_NDEBUG is not defined, but this appears to be broken
too.

MDEPKG_NDEBUG is added to the CC flags only, never to the PP flags -
given that a #define is fundamentally a PP flag, it would be better if
all -D flags were carried in a separate variable that gets added to
both, but this is future refactoring that I won't get around to
myself, most probably.

Leif, any thoughts?


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118670): https://edk2.groups.io/g/devel/message/118670
Mute This Topic: https://groups.io/mt/105977470/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-