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 <[email protected]> >> --- >> 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?
The blurb does say, > This aligns X64 with IA32, which is already built using -Os > for both DEBUG and RELEASE. and indeed I can see -Os in *_GCC44_IA32_CC_FLAGS. So maybe we've never had source level debugging for Ia32 binaries. The few times I used gdb, I certainly looked at X64 binaries only. I guess I'm no longer in a position to suggest anything, but I'd like to hear comments. If the consensus is to stick with this v2 series as-is, and *maybe* add NOOPT sometime later, I won't contest it. I'll go ahead and build-test this series in that case. Comments? :) Thanks Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
