Re: [edk2] [PATCH v2 3/5] BaseTools/tools_def: enable Os optimization for GCC X64 builds

[Paolo Bonzini]

On 16/07/2016 14:29, Laszlo Ersek wrote:
> 
> However, I recall from the thread that -Os enables -fomit-frame-pointer,
> which might make source level debugging impossible (according to the GCC
> manual).

This is only with very old debuggers.  Current debuggers use DWARF
annotations which support generation of backtraces even in the lack of a
frame pointer.

That said, the best debugging experience on recent GCC (4.8 I think) is
with -Og if you want to use it for DEBUG builds.  -O1 and -Os are
probably the best for firmware though, where code size is important.

Paolo

> Now, we're not big on source level debugging in GCC builds, at least
> right now, plus I also cannot claim that that -fomit-frame-pointer is
> never enabled *otherwise*. Much as I know -fomit-frame-pointer could be
> enabled with -O1, -O2, even with -O0?...
> 
> I'd just like to avoid a setting that *guarantees* that source level
> debugging would be impossible or garbled. Ard, can you comment on that?
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 3/5] BaseTools/tools_def: enable Os optimization for GCC X64 builds

[Kinney, Michael D]

Hi Giri,

Responses below.

Mike

> -Original Message-
> From: Mudusuru, Giri P
> Sent: Sunday, July 17, 2016 9:58 AM
> To: Laszlo Ersek <ler...@redhat.com>; Kinney, Michael D 
> <michael.d.kin...@intel.com>;
> Ard Biesheuvel <ard.biesheu...@linaro.org>
> Cc: Bruce Cran <br...@cran.org.uk>; Scott Duplichan <sc...@notabs.org>; 
> Justen, Jordan
> L <jordan.l.jus...@intel.com>; edk2-devel@lists.01.org 
> <edk2-de...@ml01.01.org>;
> af...@apple.com; Gao, Liming <liming@intel.com>; Paolo Bonzini
> <pbonz...@redhat.com>
> Subject: RE: [edk2] [PATCH v2 3/5] BaseTools/tools_def: enable Os 
> optimization for GCC
> X64 builds
> 
> Here is the summary of discussion I understand from this thread
> 
> -RELEASE - Debug logs are OFF, Optimization ON, Symbolic debug OFF
> -DEBUG - Debug logs are ON, Optimization ON, Symbolic debug ON
> -NOOPT - Debug logs are ON, Optimization OFF, Symbolic debug ON

These are BUILDTARGET names.  Logging is an independent feature from these 
BUILDTARGET names.  A BUILDTARGET is specified using the -b flag to build.exe 
and selects compiler/linker flags from Conf/tools_def.txt.

> 
> Use case for enabling symbolic debug on release mode could be during debug of 
> timing
> related bugs where enabling debug logs resolves the issues. In this use case 
> we might
> want Symbolic debug on RELEASE build also.

Since LOGGING is an independent feature, you would debug this with DEBUG 
enabled.

> 
> Use case for disable optimization on release mode is to debug some compiler
> optimization issues, in which we might need to disable optimization in 
> release mode.

This would only be a temporary workaround.  You should always root cause 
failures that
only show up when optimization is enabled.  It usually indicates a logic bug or 
race
condition that needs to be resolved.

> 
> As long as we have independent control of Debug Logs, Optimization and 
> Symbolic debug
> we can have any possible combination.

Agreed.

> 
> How about we have -RELEASE and -DEBUG (same as discussed and most commonly 
> used)
> 
> -RELEASE - Debug logs are OFF, Optimization ON, Symbolic debug OFF
> -DEBUG - Debug logs are ON, Optimization ON, Symbolic debug ON

I disagree.  DEBUG as a BUILDTARGET means symbolic debug.  The use of DEBUG to 
also 
mean logging has been a practice I have observed for platforms, but that is 
overloading
the meaning of a BUILDTARGET that is used to select different compiler/linker 
flags.  I recommend adding a flag called LOGGING so LOGGING can be 
enabled/disabled
independent of symbolic debug.

> 
> and independent flags for -NOOPT and -SOURCE_DEBUG (Symbolic debug) as two 
> independent
> options for special debug purposes?

SOURCE_DEBUG_ENABLE is yet another independent feature.  DEBUG means symbols 
should
be generated and the PE/COFF image should contains a debug directory entry that 
is a 
file path to the symbol file (e.g. PDB for MSFT ) on the host system.  

When the UDK Debugger Tool is used to support source level debug using WinDbg 
or GDB
and a UART or other serial stream device between a host and target, a debug 
agent
must be built into the target FW image.  SOURCE_DEBUG_ENABLE is the feature 
name 
that can be used to enable/disable the addition of the debug agent.

So in the end, there really are 5 flags involved to get all the combinations

Build.exe -b flag (pick one of)
DEBUG   
RELEASE 
NOOPT

Define names (all 4 combinations supported)
LOGGING
SOURCE_DEBUG_ENABLE

This means there are 12 combinations of these flags.  It does not make sense
to use a BUILDTARGET of RELEASE with SOURCE_DEBUG_ENABLE set. So 10 combinations
are meaningful.

BUILDTARGET LOGGING SOURCE_DEBUG_ENABLE
===   ===
RELEASEDisabled   Disabled No log messages and compiler 
optimizations enabled.  
RELEASEDisabled   Enabled  N/A (UDK Debugger Tool should 
work, but would only show disassembly without symbols)
RELEASEEnabledDisabled Logging messages enabled and 
compiler optimizations enabled.
RELEASEEnabledEnabled  N/A (UDK Debugger Tool should 
work, but would only show disassembly without symbols)
DEBUG Disabled   Disabled Symbolic debug using JTAG.  
Compiler optimizations enabled.
DEBUG  Disabled   Enabled  Symbolic debug using the UDK 
Debugger Tool.  Compiler Optimizations enabled.
DEBUG EnabledDisabled Symbolic debug using JTAG 
with log messages.  Compiler Optimizations enabled.
DEBUG EnabledEnabled  Symbolic debug using the UDK 
Debugger Tool with log messages.  Compiler Optimizations enabled.
NOOPT Disabled   Disabled   

Re: [edk2] [PATCH v2 3/5] BaseTools/tools_def: enable Os optimization for GCC X64 builds

[Mudusuru, Giri P]

Here is the summary of discussion I understand from this thread
 
-RELEASE - Debug logs are OFF, Optimization ON, Symbolic debug OFF
-DEBUG - Debug logs are ON, Optimization ON, Symbolic debug ON
-NOOPT - Debug logs are ON, Optimization OFF, Symbolic debug ON

Use case for enabling symbolic debug on release mode could be during debug of 
timing related bugs where enabling debug logs resolves the issues. In this use 
case we might want Symbolic debug on RELEASE build also.

Use case for disable optimization on release mode is to debug some compiler 
optimization issues, in which we might need to disable optimization in release 
mode.

As long as we have independent control of Debug Logs, Optimization and Symbolic 
debug we can have any possible combination.

How about we have -RELEASE and -DEBUG (same as discussed and most commonly used)

-RELEASE - Debug logs are OFF, Optimization ON, Symbolic debug OFF
-DEBUG - Debug logs are ON, Optimization ON, Symbolic debug ON

and independent flags for -NOOPT and -SOURCE_DEBUG (Symbolic debug) as two 
independent options for special debug purposes? 

Thanks,
-Giri 

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Saturday, July 16, 2016 11:22 AM
> To: Kinney, Michael D <michael.d.kin...@intel.com>; Ard Biesheuvel
> <ard.biesheu...@linaro.org>
> Cc: Bruce Cran <br...@cran.org.uk>; Scott Duplichan <sc...@notabs.org>;
> Justen, Jordan L <jordan.l.jus...@intel.com>; edk2-devel@lists.01.org
> <edk2-de...@ml01.01.org>; af...@apple.com; Gao, Liming
> <liming@intel.com>; Paolo Bonzini <pbonz...@redhat.com>
> Subject: Re: [edk2] [PATCH v2 3/5] BaseTools/tools_def: enable Os
> optimization for GCC X64 builds
> 
> On 07/16/16 19:43, Kinney, Michael D wrote:
> > Laszlo,
> >
> > Symbolic debugging should be fully supported at all optimization levels.
> The
> > compiler/linker generates .pdb files for MSFT and .debug sections for GCC.
> >
> > The purpose of NOOPT is not to support source level debug.  It is to make
> > debug easier.  When optimizations are turned up, many of the call
> parameters and
> > local variables can may be optimized into registers and calls can be 
> > inlined.
> > Also, the same register may be used for multiple parameters or locals
> depending
> > on how they are used in the function.  Not all debuggers are aware of
> these
> > register optimizations and may show incorrect values for parameters and
> locals.
> 
> I may not have used the correct terminology, but the case when -b DEBUG
> enables the intrusive optimizations that you describe (i.e., the source
> code cannot be uniquely matched to the firmware state) is practically
> undistinguishable from the case when no debug symbols exist at all. It
> is unusable for analysis with sufficient detail.
> 
> In that sense, to me at least, the difference between DEBUG and NOOPT is
> not "symbolic debugging is hard or easy"; it is "symbolic debugging is
> unusable (as if the symbols don't exist) vs. fully supported".
> 
> Like everyone else, I've debugged normal Linux userspace processes that
> were built with "-g -O2", and their core dumps are useless for any
> efficient purposes. One is able to narrow it down to a more or less
> tight function context, but for understanding local variables, one has
> to look at registers and disassembly (and even that isn't guaranteed to
> produce results). Single stepping simply doesn't work; gcc can routinely
> reorder the assembly so that it loses any resemblance with the original
> code (the Linux kernel disables some of these gcc optimization features
> individually). So, as far as I'm concerned, symbolic debugging is
> entirely defeated by -O2 or -Os, regardless of -g. This is why I equate
> 
> - NOOPT to "symbolic debug works"
> - DEBUG to "symbolic debug doesn't work, but ASSERT() and friends do"
> - RELEASE to "neither of those work".
> 
> > When a difficult bug is being evaluated, it is sometimes easier to make sure
> > these register optimization are disabled and function inlining id disables
> > so the debugger can show correct values for parameters and locals on
> every call
> > in the call stack.
> 
> This is something that I expect by default from a binary whose build
> options are supposed to support symbolic debugging.
> 
> > In this case, a single module under debug may disable
> > optimization in DSC  or INF [BuildOptions], or if all modules
> > need optimization disabled to debug across the entire call stack, NOOPT
> > can be used.
> 
> I accept that your description defines the officia

Re: [edk2] [PATCH v2 3/5] BaseTools/tools_def: enable Os optimization for GCC X64 builds

[Laszlo Ersek]

On 07/16/16 19:43, Kinney, Michael D wrote:
> Laszlo,
> 
> Symbolic debugging should be fully supported at all optimization levels.  The
> compiler/linker generates .pdb files for MSFT and .debug sections for GCC.
> 
> The purpose of NOOPT is not to support source level debug.  It is to make
> debug easier.  When optimizations are turned up, many of the call parameters 
> and
> local variables can may be optimized into registers and calls can be inlined.
> Also, the same register may be used for multiple parameters or locals 
> depending
> on how they are used in the function.  Not all debuggers are aware of these 
> register optimizations and may show incorrect values for parameters and 
> locals.

I may not have used the correct terminology, but the case when -b DEBUG
enables the intrusive optimizations that you describe (i.e., the source
code cannot be uniquely matched to the firmware state) is practically
undistinguishable from the case when no debug symbols exist at all. It
is unusable for analysis with sufficient detail.

In that sense, to me at least, the difference between DEBUG and NOOPT is
not "symbolic debugging is hard or easy"; it is "symbolic debugging is
unusable (as if the symbols don't exist) vs. fully supported".

Like everyone else, I've debugged normal Linux userspace processes that
were built with "-g -O2", and their core dumps are useless for any
efficient purposes. One is able to narrow it down to a more or less
tight function context, but for understanding local variables, one has
to look at registers and disassembly (and even that isn't guaranteed to
produce results). Single stepping simply doesn't work; gcc can routinely
reorder the assembly so that it loses any resemblance with the original
code (the Linux kernel disables some of these gcc optimization features
individually). So, as far as I'm concerned, symbolic debugging is
entirely defeated by -O2 or -Os, regardless of -g. This is why I equate

- NOOPT to "symbolic debug works"
- DEBUG to "symbolic debug doesn't work, but ASSERT() and friends do"
- RELEASE to "neither of those work".

> When a difficult bug is being evaluated, it is sometimes easier to make sure
> these register optimization are disabled and function inlining id disables
> so the debugger can show correct values for parameters and locals on every 
> call
> in the call stack.

This is something that I expect by default from a binary whose build
options are supposed to support symbolic debugging.

> In this case, a single module under debug may disable 
> optimization in DSC  or INF [BuildOptions], or if all modules
> need optimization disabled to debug across the entire call stack, NOOPT 
> can be used.

I accept that your description defines the official meaning for NOOPT /
DEBUG / RELEASE, and I thank you for educating me on them. For practical
purposes though, I'll have to stick with my (non-official) definitions
-- if we add -Os to DEBUG (and I don't mind if we do), then to me
personally, it won't be suitable for source level debugging. Only NOOPT
will be (to be added later).

Thanks!
Laszlo

> Best regards,
> 
> Mike
> 
>> -Original Message-
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Saturday, July 16, 2016 7:45 AM
>> To: Ard Biesheuvel 
>> Cc: edk2-devel@lists.01.org ; af...@apple.com; Gao, 
>> Liming
>> ; Shi, Steven ; Zhu, Yonghong
>> ; Kinney, Michael D ; 
>> Justen,
>> Jordan L ; Bruce Cran ; Paolo 
>> Bonzini
>> ; Scott Duplichan 
>> Subject: Re: [PATCH v2 3/5] BaseTools/tools_def: enable Os optimization for 
>> GCC X64
>> builds
>>
>> On 07/16/16 14:58, Ard Biesheuvel wrote:
>>
>>> Bottom line is that I don't really care :-) -Os for RELEASE is a clear
>>> improvement. If nobody is doing source code level debugging using GCC
>>> builds, it appears to be an improvement for DEBUG as well. In any
>>> case, it would be good to have the numbers so we can make an informed
>>> decision.
>>
>> At this point you've sort of convinced me that we should add -Os to
>> DEBUG as well. It *doubly* aligns DEBUG_GCCxx_X64_CC_FLAGS with the
>> status quo: first with GCC+IA32, second with non-GCC+X64.
>>
>> The gdb setup for GCC+X64 is so contrived at the moment *anyway* that
>> removing -Os from the build flags as a further step is practically no
>> additional burden. If we become serious about it, we can always
>> introduce NOOPT later, further aligning GCC with other toolchains on
>> IA32 and X64.
>>
>>> Another thing I noticed: OpensslLib uses -UNO_BUILTIN_VA_ARGS to
>>> switch to the default va_list implementation, which is necessary since
>>> its variadic functions lack an EFIAPI annotation. This means I should
>>> probably revise the patch to allow the standard __builtins to be used,
>>> e.g., add 

Re: [edk2] [PATCH v2 3/5] BaseTools/tools_def: enable Os optimization for GCC X64 builds

[Kinney, Michael D]

Laszlo,

Symbolic debugging should be fully supported at all optimization levels.  The
compiler/linker generates .pdb files for MSFT and .debug sections for GCC.

The purpose of NOOPT is not to support source level debug.  It is to make
debug easier.  When optimizations are turned up, many of the call parameters and
local variables can may be optimized into registers and calls can be inlined.
Also, the same register may be used for multiple parameters or locals depending
on how they are used in the function.  Not all debuggers are aware of these 
register optimizations and may show incorrect values for parameters and locals.

When a difficult bug is being evaluated, it is sometimes easier to make sure
these register optimization are disabled and function inlining id disables
so the debugger can show correct values for parameters and locals on every call
in the call stack.  In this case, a single module under debug may disable 
optimization in DSC  or INF [BuildOptions], or if all modules
need optimization disabled to debug across the entire call stack, NOOPT 
can be used.

Best regards,

Mike

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Saturday, July 16, 2016 7:45 AM
> To: Ard Biesheuvel 
> Cc: edk2-devel@lists.01.org ; af...@apple.com; Gao, 
> Liming
> ; Shi, Steven ; Zhu, Yonghong
> ; Kinney, Michael D ; 
> Justen,
> Jordan L ; Bruce Cran ; Paolo 
> Bonzini
> ; Scott Duplichan 
> Subject: Re: [PATCH v2 3/5] BaseTools/tools_def: enable Os optimization for 
> GCC X64
> builds
> 
> On 07/16/16 14:58, Ard Biesheuvel wrote:
> 
> > Bottom line is that I don't really care :-) -Os for RELEASE is a clear
> > improvement. If nobody is doing source code level debugging using GCC
> > builds, it appears to be an improvement for DEBUG as well. In any
> > case, it would be good to have the numbers so we can make an informed
> > decision.
> 
> At this point you've sort of convinced me that we should add -Os to
> DEBUG as well. It *doubly* aligns DEBUG_GCCxx_X64_CC_FLAGS with the
> status quo: first with GCC+IA32, second with non-GCC+X64.
> 
> The gdb setup for GCC+X64 is so contrived at the moment *anyway* that
> removing -Os from the build flags as a further step is practically no
> additional burden. If we become serious about it, we can always
> introduce NOOPT later, further aligning GCC with other toolchains on
> IA32 and X64.
> 
> > Another thing I noticed: OpensslLib uses -UNO_BUILTIN_VA_ARGS to
> > switch to the default va_list implementation, which is necessary since
> > its variadic functions lack an EFIAPI annotation. This means I should
> > probably revise the patch to allow the standard __builtins to be used,
> > e.g., add -DNO_MS_ABI_VARARGS to OpensslLib instead, and make the use
> > of __builtin_ms_va_list conditional on !defined(NO_MS_ABI_VARARGS)
> 
> Aaargh. I've run into (independent) varargs problems with OpenSSL in
> edk2 before, so I'm not sure how my testing missed this!
> 
> Ah wait, I may know how -- I think I wanted to use EnrollDefaultKeys.efi
> as a starting point for SB testing too, but I didn't get as far with it,
> because -O2 in your v1 triggered a latent bug in the app.
> 
> ... So, with your next update, we won't just distinguish "builtin" from
> "no-builtin" for VA_LIST, we'll also distinguish "MS" from "SYSV" within
> "builtin:. :(
> 
>  just got twice as
> annoying. :( :(
> 
> I guess I'll delay my testing until your v3. Is that okay with you?
> 
> Thanks
> Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 3/5] BaseTools/tools_def: enable Os optimization for GCC X64 builds

[Laszlo Ersek]

On 07/16/16 14:58, Ard Biesheuvel wrote:

> Bottom line is that I don't really care :-) -Os for RELEASE is a clear
> improvement. If nobody is doing source code level debugging using GCC
> builds, it appears to be an improvement for DEBUG as well. In any
> case, it would be good to have the numbers so we can make an informed
> decision.

At this point you've sort of convinced me that we should add -Os to
DEBUG as well. It *doubly* aligns DEBUG_GCCxx_X64_CC_FLAGS with the
status quo: first with GCC+IA32, second with non-GCC+X64.

The gdb setup for GCC+X64 is so contrived at the moment *anyway* that
removing -Os from the build flags as a further step is practically no
additional burden. If we become serious about it, we can always
introduce NOOPT later, further aligning GCC with other toolchains on
IA32 and X64.

> Another thing I noticed: OpensslLib uses -UNO_BUILTIN_VA_ARGS to
> switch to the default va_list implementation, which is necessary since
> its variadic functions lack an EFIAPI annotation. This means I should
> probably revise the patch to allow the standard __builtins to be used,
> e.g., add -DNO_MS_ABI_VARARGS to OpensslLib instead, and make the use
> of __builtin_ms_va_list conditional on !defined(NO_MS_ABI_VARARGS)

Aaargh. I've run into (independent) varargs problems with OpenSSL in
edk2 before, so I'm not sure how my testing missed this!

Ah wait, I may know how -- I think I wanted to use EnrollDefaultKeys.efi
as a starting point for SB testing too, but I didn't get as far with it,
because -O2 in your v1 triggered a latent bug in the app.

... So, with your next update, we won't just distinguish "builtin" from
"no-builtin" for VA_LIST, we'll also distinguish "MS" from "SYSV" within
"builtin:. :(

 just got twice as
annoying. :( :(

I guess I'll delay my testing until your v3. Is that okay with you?

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 3/5] BaseTools/tools_def: enable Os optimization for GCC X64 builds

[Ard Biesheuvel]

On 16 July 2016 at 14:29, Laszlo Ersek  wrote:
> On 07/16/16 00:16, Ard Biesheuvel wrote:
>> Now that we switched to the __builtin_ms_va_list VA_LIST type for
>> GCC/X64, we can trust the compiler to do the right thing even under
>> optimization, and so we can enable -Os optimization all the way back
>> to GCC44, and drop the -D define that prevents the use of the __builtin
>> VA_LIST types. Note that this requires the -maccumulate-outgoing-args
>> switch as well.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel 
>> ---
>>  BaseTools/Conf/tools_def.template | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/BaseTools/Conf/tools_def.template 
>> b/BaseTools/Conf/tools_def.template
>> index 2065fa34998f..a7da6741611d 100644
>> --- a/BaseTools/Conf/tools_def.template
>> +++ b/BaseTools/Conf/tools_def.template
>> @@ -4353,7 +4353,7 @@ DEFINE GCC_AARCH64_RC_FLAGS= -I binary -O 
>> elf64-littleaarch64 -B aarch64
>>
>>  DEFINE GCC44_ALL_CC_FLAGS= -g -fshort-wchar 
>> -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections 
>> -fdata-sections -c -include AutoGen.h -fno-common 
>> -DSTRING_ARRAY_NAME=$(BASE_NAME)Strings
>>  DEFINE GCC44_IA32_CC_FLAGS   = DEF(GCC44_ALL_CC_FLAGS) -m32 
>> -march=i586 -malign-double -fno-stack-protector -D EFI32 
>> -fno-asynchronous-unwind-tables
>> -DEFINE GCC44_X64_CC_FLAGS= DEF(GCC44_ALL_CC_FLAGS) -m64 
>> -fno-stack-protector "-DEFIAPI=__attribute__((ms_abi))" 
>> -DNO_BUILTIN_VA_FUNCS -mno-red-zone -Wno-address -mcmodel=large 
>> -fno-asynchronous-unwind-tables
>> +DEFINE GCC44_X64_CC_FLAGS= DEF(GCC44_ALL_CC_FLAGS) -m64 
>> -fno-stack-protector "-DEFIAPI=__attribute__((ms_abi))" -Os 
>> -maccumulate-outgoing-args -mno-red-zone -Wno-address -mcmodel=large 
>> -fno-asynchronous-unwind-tables
>>  DEFINE GCC44_IA32_X64_DLINK_COMMON   = -nostdlib -n -q --gc-sections -z 
>> common-page-size=0x20
>>  DEFINE GCC44_IA32_X64_ASLDLINK_FLAGS = DEF(GCC44_IA32_X64_DLINK_COMMON) 
>> --entry ReferenceAcpiTable -u ReferenceAcpiTable
>>  DEFINE GCC44_IA32_X64_DLINK_FLAGS= DEF(GCC44_IA32_X64_DLINK_COMMON) 
>> --entry $(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Map 
>> $(DEST_DIR_DEBUG)/$(BASE_NAME).map
>>
>
> Before I embark on build-testing this series too with my "build farm",
> I'd like to point out this thread:
>
> http://thread.gmane.org/gmane.comp.bios.tianocore.devel/10741/focus=10961
>
> Now, the assumption that -Os itself was causing the corruption has been
> laid to rest; we now know that the corruption was the product of the
> VA_LIST implementation, which is exactly what this series is replacing.
> So that's not why I'm pointing at the thread.
>
> However, I recall from the thread that -Os enables -fomit-frame-pointer,
> which might make source level debugging impossible (according to the GCC
> manual).
>
> Now, we're not big on source level debugging in GCC builds, at least
> right now, plus I also cannot claim that that -fomit-frame-pointer is
> never enabled *otherwise*. Much as I know -fomit-frame-pointer could be
> enabled with -O1, -O2, even with -O0?...
>

I simply extrapolated from IA32, which uses -Os for DEBUG as well.
Since IA32 has even fewer general purpose registers, I would assume
that by the same reasoning, this is OK for X64 as well. I wouldn't
have given it any thought if you hadn't mentioned it :-)

> I'd just like to avoid a setting that *guarantees* that source level
> debugging would be impossible or garbled. Ard, can you comment on that?
>

That seems like a genuine concern, and -Os optimization but with a
frame pointer should be perfectly feasible, so perhaps it is better to
simply add -fno-omit-frame-pointer in the DEBUG case, especially since
it does not hurt X64 as much as it hurts IA32 code.

> ... Actually, just now I'm remembering something Scott explained to me:
> the difference between DEBUG, RELEASE, and NOOPT. Both DEBUG and RELEASE
> are supposed to be optimized (they differ in the compilation of DEBUG,
> DEBUG_CODE, ASSERT etc; not in optimization). NOOPT on the other hand is
> supposed to keep DEBUGs, but also disable optimization (for source level
> debugging).
>
> At the moment, we have no NOOPT settings for GCC. We only have RELEASE
> (in the "supposed" meaning of RELEASE), and DEBUG (which has,
> traditionally, stood for the NOOPT behavior actually).
>
> Version 1 of this patch set uses -O2 instead of -Os, but another
> difference is that v1 only added optimization to RELEASE. This version
> adds optimization (-Os) to DEBUG too (*) -- I guess in no small part
> because I expressed a wish for that? --, but it doesn't introduce a
> NOOPT target. I'm concerned that this might cause us to lose any usable
> source level debugging, even though our current "source level debugging"
> facility means a super contrived, out-of-tree gdb setup.
>
> (*) This is not my 

Re: [edk2] [PATCH v2 3/5] BaseTools/tools_def: enable Os optimization for GCC X64 builds

[Laszlo Ersek]

On 07/16/16 14:29, Laszlo Ersek wrote:
> On 07/16/16 00:16, Ard Biesheuvel wrote:
>> Now that we switched to the __builtin_ms_va_list VA_LIST type for
>> GCC/X64, we can trust the compiler to do the right thing even under
>> optimization, and so we can enable -Os optimization all the way back
>> to GCC44, and drop the -D define that prevents the use of the __builtin
>> VA_LIST types. Note that this requires the -maccumulate-outgoing-args
>> switch as well.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel 
>> ---
>>  BaseTools/Conf/tools_def.template | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/BaseTools/Conf/tools_def.template 
>> b/BaseTools/Conf/tools_def.template
>> index 2065fa34998f..a7da6741611d 100644
>> --- a/BaseTools/Conf/tools_def.template
>> +++ b/BaseTools/Conf/tools_def.template
>> @@ -4353,7 +4353,7 @@ DEFINE GCC_AARCH64_RC_FLAGS= -I binary -O 
>> elf64-littleaarch64 -B aarch64
>>  
>>  DEFINE GCC44_ALL_CC_FLAGS= -g -fshort-wchar 
>> -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections 
>> -fdata-sections -c -include AutoGen.h -fno-common 
>> -DSTRING_ARRAY_NAME=$(BASE_NAME)Strings
>>  DEFINE GCC44_IA32_CC_FLAGS   = DEF(GCC44_ALL_CC_FLAGS) -m32 
>> -march=i586 -malign-double -fno-stack-protector -D EFI32 
>> -fno-asynchronous-unwind-tables
>> -DEFINE GCC44_X64_CC_FLAGS= DEF(GCC44_ALL_CC_FLAGS) -m64 
>> -fno-stack-protector "-DEFIAPI=__attribute__((ms_abi))" 
>> -DNO_BUILTIN_VA_FUNCS -mno-red-zone -Wno-address -mcmodel=large 
>> -fno-asynchronous-unwind-tables
>> +DEFINE GCC44_X64_CC_FLAGS= DEF(GCC44_ALL_CC_FLAGS) -m64 
>> -fno-stack-protector "-DEFIAPI=__attribute__((ms_abi))" -Os 
>> -maccumulate-outgoing-args -mno-red-zone -Wno-address -mcmodel=large 
>> -fno-asynchronous-unwind-tables
>>  DEFINE GCC44_IA32_X64_DLINK_COMMON   = -nostdlib -n -q --gc-sections -z 
>> common-page-size=0x20
>>  DEFINE GCC44_IA32_X64_ASLDLINK_FLAGS = DEF(GCC44_IA32_X64_DLINK_COMMON) 
>> --entry ReferenceAcpiTable -u ReferenceAcpiTable
>>  DEFINE GCC44_IA32_X64_DLINK_FLAGS= DEF(GCC44_IA32_X64_DLINK_COMMON) 
>> --entry $(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Map 
>> $(DEST_DIR_DEBUG)/$(BASE_NAME).map
>>
> 
> Before I embark on build-testing this series too with my "build farm",
> I'd like to point out this thread:
> 
> http://thread.gmane.org/gmane.comp.bios.tianocore.devel/10741/focus=10961
> 
> Now, the assumption that -Os itself was causing the corruption has been
> laid to rest; we now know that the corruption was the product of the
> VA_LIST implementation, which is exactly what this series is replacing.
> So that's not why I'm pointing at the thread.
> 
> However, I recall from the thread that -Os enables -fomit-frame-pointer,
> which might make source level debugging impossible (according to the GCC
> manual).
> 
> Now, we're not big on source level debugging in GCC builds, at least
> right now, plus I also cannot claim that that -fomit-frame-pointer is
> never enabled *otherwise*. Much as I know -fomit-frame-pointer could be
> enabled with -O1, -O2, even with -O0?...
> 
> I'd just like to avoid a setting that *guarantees* that source level
> debugging would be impossible or garbled. Ard, can you comment on that?
> 
> ... Actually, just now I'm remembering something Scott explained to me:
> the difference between DEBUG, RELEASE, and NOOPT. Both DEBUG and RELEASE
> are supposed to be optimized (they differ in the compilation of DEBUG,
> DEBUG_CODE, ASSERT etc; not in optimization). NOOPT on the other hand is
> supposed to keep DEBUGs, but also disable optimization (for source level
> debugging).
> 
> At the moment, we have no NOOPT settings for GCC. We only have RELEASE
> (in the "supposed" meaning of RELEASE), and DEBUG (which has,
> traditionally, stood for the NOOPT behavior actually).
> 
> Version 1 of this patch set uses -O2 instead of -Os, but another
> difference is that v1 only added optimization to RELEASE. This version
> adds optimization (-Os) to DEBUG too (*) -- I guess in no small part
> because I expressed a wish for that? --, but it doesn't introduce a
> NOOPT target. I'm concerned that this might cause us to lose any usable
> source level debugging, even though our current "source level debugging"
> facility means a super contrived, out-of-tree gdb setup.
> 
> (*) This is not my "discovery" of course, it's announced in the v2 blurb.
> 
> I don't really know what to ask for / wish for :) I think introducing
> NOOPT might be a sizeable task, and it would even require changes to
> platforms (OvmfPkg and ArmVirtPkg minimally). So I don't feel good about
> asking Ard to add NOOPT as well.
> 
> Instead, I admit that my suggestion (implied request?) in the
> v1 thread -- i.e., to add optimization to DEBUG -- broke the GCC
> toolchain tradition of DEBUG standing for NOOPT actually.
> 
> I'm very sorry about that. :(

Re: [edk2] [PATCH v2 3/5] BaseTools/tools_def: enable Os optimization for GCC X64 builds

[Laszlo Ersek]

On 07/16/16 00:16, Ard Biesheuvel wrote:
> Now that we switched to the __builtin_ms_va_list VA_LIST type for
> GCC/X64, we can trust the compiler to do the right thing even under
> optimization, and so we can enable -Os optimization all the way back
> to GCC44, and drop the -D define that prevents the use of the __builtin
> VA_LIST types. Note that this requires the -maccumulate-outgoing-args
> switch as well.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel 
> ---
>  BaseTools/Conf/tools_def.template | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/BaseTools/Conf/tools_def.template 
> b/BaseTools/Conf/tools_def.template
> index 2065fa34998f..a7da6741611d 100644
> --- a/BaseTools/Conf/tools_def.template
> +++ b/BaseTools/Conf/tools_def.template
> @@ -4353,7 +4353,7 @@ DEFINE GCC_AARCH64_RC_FLAGS= -I binary -O 
> elf64-littleaarch64 -B aarch64
>  
>  DEFINE GCC44_ALL_CC_FLAGS= -g -fshort-wchar -fno-strict-aliasing 
> -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -c 
> -include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=$(BASE_NAME)Strings
>  DEFINE GCC44_IA32_CC_FLAGS   = DEF(GCC44_ALL_CC_FLAGS) -m32 
> -march=i586 -malign-double -fno-stack-protector -D EFI32 
> -fno-asynchronous-unwind-tables
> -DEFINE GCC44_X64_CC_FLAGS= DEF(GCC44_ALL_CC_FLAGS) -m64 
> -fno-stack-protector "-DEFIAPI=__attribute__((ms_abi))" -DNO_BUILTIN_VA_FUNCS 
> -mno-red-zone -Wno-address -mcmodel=large -fno-asynchronous-unwind-tables
> +DEFINE GCC44_X64_CC_FLAGS= DEF(GCC44_ALL_CC_FLAGS) -m64 
> -fno-stack-protector "-DEFIAPI=__attribute__((ms_abi))" -Os 
> -maccumulate-outgoing-args -mno-red-zone -Wno-address -mcmodel=large 
> -fno-asynchronous-unwind-tables
>  DEFINE GCC44_IA32_X64_DLINK_COMMON   = -nostdlib -n -q --gc-sections -z 
> common-page-size=0x20
>  DEFINE GCC44_IA32_X64_ASLDLINK_FLAGS = DEF(GCC44_IA32_X64_DLINK_COMMON) 
> --entry ReferenceAcpiTable -u ReferenceAcpiTable
>  DEFINE GCC44_IA32_X64_DLINK_FLAGS= DEF(GCC44_IA32_X64_DLINK_COMMON) 
> --entry $(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Map 
> $(DEST_DIR_DEBUG)/$(BASE_NAME).map
> 

Before I embark on build-testing this series too with my "build farm",
I'd like to point out this thread:

http://thread.gmane.org/gmane.comp.bios.tianocore.devel/10741/focus=10961

Now, the assumption that -Os itself was causing the corruption has been
laid to rest; we now know that the corruption was the product of the
VA_LIST implementation, which is exactly what this series is replacing.
So that's not why I'm pointing at the thread.

However, I recall from the thread that -Os enables -fomit-frame-pointer,
which might make source level debugging impossible (according to the GCC
manual).

Now, we're not big on source level debugging in GCC builds, at least
right now, plus I also cannot claim that that -fomit-frame-pointer is
never enabled *otherwise*. Much as I know -fomit-frame-pointer could be
enabled with -O1, -O2, even with -O0?...

I'd just like to avoid a setting that *guarantees* that source level
debugging would be impossible or garbled. Ard, can you comment on that?

... Actually, just now I'm remembering something Scott explained to me:
the difference between DEBUG, RELEASE, and NOOPT. Both DEBUG and RELEASE
are supposed to be optimized (they differ in the compilation of DEBUG,
DEBUG_CODE, ASSERT etc; not in optimization). NOOPT on the other hand is
supposed to keep DEBUGs, but also disable optimization (for source level
debugging).

At the moment, we have no NOOPT settings for GCC. We only have RELEASE
(in the "supposed" meaning of RELEASE), and DEBUG (which has,
traditionally, stood for the NOOPT behavior actually).

Version 1 of this patch set uses -O2 instead of -Os, but another
difference is that v1 only added optimization to RELEASE. This version
adds optimization (-Os) to DEBUG too (*) -- I guess in no small part
because I expressed a wish for that? --, but it doesn't introduce a
NOOPT target. I'm concerned that this might cause us to lose any usable
source level debugging, even though our current "source level debugging"
facility means a super contrived, out-of-tree gdb setup.

(*) This is not my "discovery" of course, it's announced in the v2 blurb.

I don't really know what to ask for / wish for :) I think introducing
NOOPT might be a sizeable task, and it would even require changes to
platforms (OvmfPkg and ArmVirtPkg minimally). So I don't feel good about
asking Ard to add NOOPT as well.

Instead, I admit that my suggestion (implied request?) in the
v1 thread -- i.e., to add optimization to DEBUG -- broke the GCC
toolchain tradition of DEBUG standing for NOOPT actually.

I'm very sorry about that. :(

In order to uphold the GCC toolchain tradition for DEBUG, should we add
-Os (and whatever else -Os requires) to RELEASE only?

Thanks
Laszlo
___

[edk2] [PATCH v2 3/5] BaseTools/tools_def: enable Os optimization for GCC X64 builds

[Ard Biesheuvel]

Now that we switched to the __builtin_ms_va_list VA_LIST type for
GCC/X64, we can trust the compiler to do the right thing even under
optimization, and so we can enable -Os optimization all the way back
to GCC44, and drop the -D define that prevents the use of the __builtin
VA_LIST types. Note that this requires the -maccumulate-outgoing-args
switch as well.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
---
 BaseTools/Conf/tools_def.template | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/BaseTools/Conf/tools_def.template 
b/BaseTools/Conf/tools_def.template
index 2065fa34998f..a7da6741611d 100644
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -4353,7 +4353,7 @@ DEFINE GCC_AARCH64_RC_FLAGS= -I binary -O 
elf64-littleaarch64 -B aarch64
 
 DEFINE GCC44_ALL_CC_FLAGS= -g -fshort-wchar -fno-strict-aliasing 
-Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -c -include 
AutoGen.h -fno-common -DSTRING_ARRAY_NAME=$(BASE_NAME)Strings
 DEFINE GCC44_IA32_CC_FLAGS   = DEF(GCC44_ALL_CC_FLAGS) -m32 
-march=i586 -malign-double -fno-stack-protector -D EFI32 
-fno-asynchronous-unwind-tables
-DEFINE GCC44_X64_CC_FLAGS= DEF(GCC44_ALL_CC_FLAGS) -m64 
-fno-stack-protector "-DEFIAPI=__attribute__((ms_abi))" -DNO_BUILTIN_VA_FUNCS 
-mno-red-zone -Wno-address -mcmodel=large -fno-asynchronous-unwind-tables
+DEFINE GCC44_X64_CC_FLAGS= DEF(GCC44_ALL_CC_FLAGS) -m64 
-fno-stack-protector "-DEFIAPI=__attribute__((ms_abi))" -Os 
-maccumulate-outgoing-args -mno-red-zone -Wno-address -mcmodel=large 
-fno-asynchronous-unwind-tables
 DEFINE GCC44_IA32_X64_DLINK_COMMON   = -nostdlib -n -q --gc-sections -z 
common-page-size=0x20
 DEFINE GCC44_IA32_X64_ASLDLINK_FLAGS = DEF(GCC44_IA32_X64_DLINK_COMMON) 
--entry ReferenceAcpiTable -u ReferenceAcpiTable
 DEFINE GCC44_IA32_X64_DLINK_FLAGS= DEF(GCC44_IA32_X64_DLINK_COMMON) 
--entry $(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Map 
$(DEST_DIR_DEBUG)/$(BASE_NAME).map
-- 
1.9.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel