diff mbox

[edk2] StdLib: the long standing build error

Message ID 53E23593.1030206@redhat.com
State New
Headers show

Commit Message

Laszlo Ersek Aug. 6, 2014, 2:02 p.m. UTC
On 08/06/14 00:31, Olivier Martin wrote:
> Why do not put '*va_arg(ap, long double *) = res;' which seems to be the valid solution and add a #ifdef for the faulty toolchains.
> I do not feel fair to keep a workaround by default when it blocks all the other toolchains.

Would the attached patch work?

(Actually, since it is very trivial, I'm quite sure it has been
investigated before. So I'll reformulate: why isn't the attached
approach acceptable?)

Thanks
Laszlo
------------------------------------------------------------------------------
Infragistics Professional
Build stunning WinForms apps today!
Reboot your WinForms applications with our WinForms controls. 
Build a bridge from your legacy apps to the future.
http://pubads.g.doubleclick.net/gampad/clk?id=153845071&iu=/4140/ostg.clktrk

Comments

Andrew Fish Aug. 6, 2014, 3:03 p.m. UTC | #1
On Aug 6, 2014, at 7:02 AM, Laszlo Ersek <lersek@redhat.com> wrote:

> On 08/06/14 00:31, Olivier Martin wrote:
>> Why do not put '*va_arg(ap, long double *) = res;' which seems to be the valid solution and add a #ifdef for the faulty toolchains.
>> I do not feel fair to keep a workaround by default when it blocks all the other toolchains.
> 
> Would the attached patch work?
> 
> (Actually, since it is very trivial, I'm quite sure it has been
> investigated before. So I'll reformulate: why isn't the attached
> approach acceptable?)
> 

Laszlo,

I’d recommend using the compiler defined value and adding a comment on why it is needed:
#if defined(_MSC_EXTENSIONS)
and not adding a new #define for this. 

The code in the #ifdef assumes a specific compiler implementation of va_list and thus is totally non portable and tied a specific compiler. 

_MSC_EXTENSIONS  is used in other places in the edk2 to deal with compiler specifics: https://svn.code.sf.net/p/edk2/code/trunk/edk2/MdePkg/Include/X64/ProcessorBind.h

Thanks,

Andrew Fish

> Thanks
> Laszlo
> 
> <0001-StdLib-LibC-Stdio-special-case-long-double-parsing-o.patch>------------------------------------------------------------------------------
> Infragistics Professional
> Build stunning WinForms apps today!
> Reboot your WinForms applications with our WinForms controls. 
> Build a bridge from your legacy apps to the future.
> http://pubads.g.doubleclick.net/gampad/clk?id=153845071&iu=/4140/ostg.clktrk_______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
------------------------------------------------------------------------------
Infragistics Professional
Build stunning WinForms apps today!
Reboot your WinForms applications with our WinForms controls. 
Build a bridge from your legacy apps to the future.
http://pubads.g.doubleclick.net/gampad/clk?id=153845071&iu=/4140/ostg.clktrk
Laszlo Ersek Aug. 6, 2014, 3:43 p.m. UTC | #2
On 08/06/14 17:03, Andrew Fish wrote:
> 
> On Aug 6, 2014, at 7:02 AM, Laszlo Ersek <lersek@redhat.com
> <mailto:lersek@redhat.com>> wrote:
> 
>> On 08/06/14 00:31, Olivier Martin wrote:
>>> Why do not put '*va_arg(ap, long double *) = res;' which seems to be
>>> the valid solution and add a #ifdef for the faulty toolchains.
>>> I do not feel fair to keep a workaround by default when it blocks all
>>> the other toolchains.
>>
>> Would the attached patch work?
>>
>> (Actually, since it is very trivial, I'm quite sure it has been
>> investigated before. So I'll reformulate: why isn't the attached
>> approach acceptable?)
>>
> 
> Laszlo,
> 
> I’d recommend using the compiler defined value and adding a comment on
> why it is needed:
> 
> #if defined(_MSC_EXTENSIONS)
> 
> and not adding a new #define for this. 
> 
> The code in the #ifdef assumes a specific compiler implementation of
> va_list and thus is totally non portable and tied a specific compiler. 
> 
> _MSC_EXTENSIONS  is used in other places in the edk2 to deal with
> compiler
> specifics: https://svn.code.sf.net/p/edk2/code/trunk/edk2/MdePkg/Include/X64/ProcessorBind.h

That's awesome. I was incredulous that no such toolchain-dependent code
existed yet, but my searches for "MSFT" and "MSVC" in source files lead
to no results. It's great that the right macro,  _MSC_EXTENSIONS, is
already there. The question is now if a toolchain ifdef has been
considered before, and if so, why wasn't accepted. If I can see it
right, Olivier started the thread about one year ago.

Thank you!
Laszlo


------------------------------------------------------------------------------
Infragistics Professional
Build stunning WinForms apps today!
Reboot your WinForms applications with our WinForms controls. 
Build a bridge from your legacy apps to the future.
http://pubads.g.doubleclick.net/gampad/clk?id=153845071&iu=/4140/ostg.clktrk
Olivier Martin Aug. 6, 2014, 4:02 p.m. UTC | #3
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: 06 August 2014 16:44
> To: edk2-devel@lists.sourceforge.net
> Cc: Mitchell, Mark L; Harry Liebel
> Subject: Re: [edk2] StdLib: the long standing build error
> 
> 
> That's awesome. I was incredulous that no such toolchain-dependent code
> existed yet, but my searches for "MSFT" and "MSVC" in source files lead
> to no results. It's great that the right macro,  _MSC_EXTENSIONS, is
> already there. The question is now if a toolchain ifdef has been
> considered before, and if so, why wasn't accepted. If I can see it
> right, Olivier started the thread about one year ago.
> 

Two years ago! The original patch was sent on 25/06/2012.




------------------------------------------------------------------------------
Infragistics Professional
Build stunning WinForms apps today!
Reboot your WinForms applications with our WinForms controls. 
Build a bridge from your legacy apps to the future.
http://pubads.g.doubleclick.net/gampad/clk?id=153845071&iu=/4140/ostg.clktrk
diff mbox

Patch

From a69752d14f0b4cdfc9dd095a36464dc7b86cda65 Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Wed, 6 Aug 2014 15:57:17 +0200
Subject: [PATCH] StdLib/LibC/Stdio: special case long double parsing on MSFT

The Microsoft compiler does not really support (long double).

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 StdLib/LibC/Stdio/Stdio.inf | 1 +
 StdLib/LibC/Stdio/vfscanf.c | 7 +++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/StdLib/LibC/Stdio/Stdio.inf b/StdLib/LibC/Stdio/Stdio.inf
index a61a558..8479dc0 100644
--- a/StdLib/LibC/Stdio/Stdio.inf
+++ b/StdLib/LibC/Stdio/Stdio.inf
@@ -145,3 +145,4 @@ 
 #
 [BuildOptions]
   GCC:*_*_*_CC_FLAGS    = -fno-builtin -Wno-pointer-to-int-cast -Wno-int-to-pointer-cast -Wno-format
+  MSFT:*_*_*_CC_FLAGS   = /D MSFT_LONG_DOUBLE
diff --git a/StdLib/LibC/Stdio/vfscanf.c b/StdLib/LibC/Stdio/vfscanf.c
index 4f84a39..414c6f4 100644
--- a/StdLib/LibC/Stdio/vfscanf.c
+++ b/StdLib/LibC/Stdio/vfscanf.c
@@ -843,12 +843,15 @@  literal:
         goto match_failure;
       if ((flags & SUPPRESS) == 0) {
         if (flags & LONGDBL) {
-          long double **mp = (long double **)ap;
           long double res = strtold(buf, &p);
+#if MSFT_LONG_DOUBLE
+          long double **mp = (long double **)ap;
 
           *(*mp) = res;
           ap += sizeof(long double *);
-/*???*/   //*va_arg(ap, long double *) = res;
+#else
+          *va_arg(ap, long double *) = res;
+#endif
         } else if (flags & LONG) {
           double res = strtod(buf, &p);
           *va_arg(ap, double *) = res;
-- 
1.8.3.1