rpms / redhat-rpm-config

Clone
Source Code
GIT

#231 Enable frame pointers by default
Merged 8 months ago by ngompa. Opened 8 months ago by dcavalca.
rpms/ dcavalca/redhat-rpm-config frame-pointers-default  into  rawhide

file modified
+15 -5 
@@ -304,8 +304,15 @@ 
 

  

  ### Frame pointers
 

  

- Frame pointers will be included by default if the `%_include_frame_pointers`
 

- macro is defined.
 

+ Frame pointers will be included by default via the `%_include_frame_pointers`
 

+ macro. To opt out, the best way is to undefine the macro. Include this in the
 

+ spec file:
 

+ 

+     %undefine _include_frame_pointers
 

+ 

+ Note that opting out might still result in frame pointers being included on
 

+ architectures where they are part of the ABI (e.g. aarch64) depending on
 

+ compiler defaults.
 

  

  ### Post-build ELF object processing
 

  
@@ -482,9 +489,12 @@ 
 

  watermarks are currently disabled on armhpf, and with the `clang`
 

  toolchain.
 

  

- If frame pointers are enabled by default (via `%_include_frame_pointers),
 

- the `-fno-omit-frame-pointer` and `-mno-omit-leaf-frame-pointer` flags will
 

- be added.
 

+ If frame pointers are enabled by default (via `%_include_frame_pointers`),
 

+ the `-fno-omit-frame-pointer` will be added on all architectures. Additional
 

+ flags will be added on specific architectures:
 

+ 

+ * `-mno-omit-leaf-frame-pointer` on x86_64 and aarch64
 

+ * `-mbackchain` on s390x
 

  

  ### Architecture-specific compiler flags

file modified
+7 -4 
@@ -336,9 +336,12 @@ 
 

  

  # Always include frame pointer information
 

  # https://fedoraproject.org/wiki/Changes/fno-omit-frame-pointer
 

- # Use "%define _include_frame_pointers 1" to enable.
 

- #%_include_frame_pointers 1
 

- %_frame_pointers_cflags %{?_include_frame_pointers:-fno-omit-frame-pointer -mno-omit-leaf-frame-pointer}
 

+ # Use "%undefine _include_frame_pointers" to disable.
 

+ %_include_frame_pointers 1
 

+ %_frame_pointers_cflags %{?_include_frame_pointers:-fno-omit-frame-pointer}
 

+ %_frame_pointers_cflags_x86_64 %{?_include_frame_pointers:-mno-omit-leaf-frame-pointer}
 

+ %_frame_pointers_cflags_aarch64 %{?_include_frame_pointers:-mno-omit-leaf-frame-pointer}
 

+ %_frame_pointers_cflags_s390x %{?_include_frame_pointers:-mbackchain}
 

  

  # Fail linking if there are undefined symbols.  Required for proper
 

  # ELF symbol versioning support.  Disabled by default.
 
@@ -381,7 +384,7 @@ 
 

  # If they are needed then add "%define _legacy_common_support 1" to the spec file.
 

  %_legacy_options	%{?_legacy_common_support: -fcommon}
 

  

- %__global_compiler_flags %{_general_options} %{_warning_options} %{_preprocessor_defines} %{_hardened_cflags} %{_annotation_cflags} %{_frame_pointers_cflags} %{_legacy_options}
 

+ %__global_compiler_flags %{_general_options} %{_warning_options} %{_preprocessor_defines} %{_hardened_cflags} %{_annotation_cflags} %{_legacy_options}
 

  

  # Automatically trim changelog entries after 2 years
 

  %_changelog_trimage	%{expr:2*365*24*60*60}

file modified
+5 -1 
@@ -4,7 +4,7 @@ 
 

  # 2) When making changes, increment the version (in baserelease) by 1.
 

  #    rpmdev-bumpspec and other tools update the macro below, which is used
 

  #    in Version: to get the desired effect.
 

- %global baserelease 238
 

+ %global baserelease 239
 

  

  Summary: Red Hat specific rpm configuration files
 

  Name: redhat-rpm-config
 
@@ -221,6 +221,10 @@ 
 

  %doc buildflags.md
 

  

  %changelog
 

+ * Wed Jan  4 2023 Davide Cavalca <dcavalca@fedoraproject.org> - 239-1
 

+ - Enable frame pointers by default
 

+ - Set arch specific flags for frame pointers support
 

+ 

  * Tue Jan  3 2023 Miro Hrončok <mhroncok@redhat.com> - 238-1
 

  - Set %%source_date_epoch_from_changelog to 1
 

  - https://fedoraproject.org/wiki/Changes/ReproducibleBuildsClampMtimes

file modified
+11 -11 
@@ -1,22 +1,22 @@ 
 

  include: /usr/lib/rpm/rpmrc
 

  

- optflags: i386 %{__global_compiler_flags} -m32 -march=i386 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection
 

- optflags: i486 %{__global_compiler_flags} -m32 -march=i486 -fasynchronous-unwind-tables -fstack-clash-protection
 

- optflags: i586 %{__global_compiler_flags} -m32 -march=i586 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection
 

- optflags: i686 %{__global_compiler_flags} -m32 -march=i686 -mtune=generic -msse2 -mfpmath=sse -mstackrealign -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection
 

- optflags: athlon %{__global_compiler_flags} -m32 -march=athlon -fasynchronous-unwind-tables -fstack-clash-protection
 

- optflags: x86_64 %{__global_compiler_flags} -m64 %{__cflags_arch_x86_64} -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection
 

+ optflags: i386 %{__global_compiler_flags} -m32 -march=i386 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection %{_frame_pointers_cflags}
 

+ optflags: i486 %{__global_compiler_flags} -m32 -march=i486 -fasynchronous-unwind-tables -fstack-clash-protection %{_frame_pointers_cflags}
 

+ optflags: i586 %{__global_compiler_flags} -m32 -march=i586 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection %{_frame_pointers_cflags}
 

+ optflags: i686 %{__global_compiler_flags} -m32 -march=i686 -mtune=generic -msse2 -mfpmath=sse -mstackrealign -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection %{_frame_pointers_cflags}
 

+ optflags: athlon %{__global_compiler_flags} -m32 -march=athlon -fasynchronous-unwind-tables -fstack-clash-protection %{_frame_pointers_cflags}
 

+ optflags: x86_64 %{__global_compiler_flags} -m64 %{__cflags_arch_x86_64} -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection %{_frame_pointers_cflags} %{_frame_pointers_cflags_x86_64}
 

  

- optflags: ppc64le %{__global_compiler_flags} -m64 %{__cflags_arch_ppc64le} -fasynchronous-unwind-tables -fstack-clash-protection
 

+ optflags: ppc64le %{__global_compiler_flags} -m64 %{__cflags_arch_ppc64le} -fasynchronous-unwind-tables -fstack-clash-protection %{_frame_pointers_cflags}
 

  

  # TODO: Remove armv7hl once f36 goes EOL.
 

- optflags: armv7hl %{__global_compiler_flags} -march=armv7-a -mfpu=vfpv3-d16 -mtune=generic-armv7-a -mabi=aapcs-linux -mfloat-abi=hard
 

+ optflags: armv7hl %{__global_compiler_flags} -march=armv7-a -mfpu=vfpv3-d16 -mtune=generic-armv7-a -mabi=aapcs-linux -mfloat-abi=hard %{_frame_pointers_cflags}
 

  

- optflags: s390x %{__global_compiler_flags} -m64 %{__cflags_arch_s390x} -fasynchronous-unwind-tables -fstack-clash-protection
 

+ optflags: s390x %{__global_compiler_flags} -m64 %{__cflags_arch_s390x} -fasynchronous-unwind-tables -fstack-clash-protection %{_frame_pointers_cflags} %{_frame_pointers_cflags_s390x}
 

  

- optflags: aarch64 %{__global_compiler_flags} -mbranch-protection=standard -fasynchronous-unwind-tables %[ "%{toolchain}" == "gcc" ? "-fstack-clash-protection" : "" ]
 

+ optflags: aarch64 %{__global_compiler_flags} -mbranch-protection=standard -fasynchronous-unwind-tables %[ "%{toolchain}" == "gcc" ? "-fstack-clash-protection" : "" ] %{_frame_pointers_cflags} %{_frame_pointers_cflags_aarch64}
 

  

- optflags: riscv64 %{__global_compiler_flags} -fasynchronous-unwind-tables %[ "%{toolchain}" == "gcc" ? "-fstack-clash-protection" : "" ]
 

+ optflags: riscv64 %{__global_compiler_flags} -fasynchronous-unwind-tables %[ "%{toolchain}" == "gcc" ? "-fstack-clash-protection" : "" ] %{_frame_pointers_cflags}
 

  

  # set build arch to fedora buildarches on hardware capable of running it
 

  # saves having to do rpmbuild --target=

file added
+7 
@@ -0,0 +1,7 @@ 
 

+ summary: Test that conditional support for frame pointers works
 

+ 

+ require:
 

+   - grep
 

+   - redhat-rpm-config
 

+   - rpm
 

+ test: ./runtest.sh

file added
+52 
@@ -0,0 +1,52 @@ 
 

+ #!/bin/sh
 

+ 

+ # Not using set -e on purpose as we manually validate the exit codes to print
 

+ # useful messages.
 

+ set -u
 

+ 

+ passed=0
 

+ failed=0
 

+ 

+ rpmeval() {
 

+   # Note: --eval needs to always be *last* here
 

+   rpm "$@" --eval='%optflags'
 

+ }
 

+ 

+ validate() {
 

+   ret=$?
 

+   if [ $ret -eq 0 ]; then
 

+     echo "PASS: $*"
 

+     passed=$((passed+1))
 

+   else
 

+     echo "FAIL: $*"
 

+     failed=$((failed+1))
 

+   fi
 

+ }
 

+ 

+ for arch in aarch64 armv7hl i386 i486 i585 i686 athlon x86_64 ppc64le s390x riscv64; do
 

+   case "$arch" in
 

+     x86_64|aarch64)
 

+       flags='-fno-omit-frame-pointer -mno-omit-leaf-frame-pointer'
 

+       ;;
 

+     s390x)
 

+       flags='-fno-omit-frame-pointer -mbackchain'
 

+       ;;
 

+     *)
 

+       flags='-fno-omit-frame-pointer'
 

+       ;;
 

+   esac
 

+ 

+   rpmeval --target="${arch}-linux" --define='%_include_frame_pointers 1' | grep -q -- "$flags"
 

+   validate "[${arch}] Test that the flags are included if the macro is defined"
 

+ 

+   rpmeval --target="${arch}-linux" --undefine='_include_frame_pointers' | grep -qv -- "$flags"
 

+   validate "[${arch}] Test that the flags are _not_ included if the macro is undefined"
 

+ 

+   rpmeval --target="${arch}-linux" | grep -q -- "$flags"
 

+   validate "[${arch}] Test that the flags are included by default"
 

+ done
 

+ 

+ echo
 

+ echo "${passed} passed, ${failed} failed"
 

+ 

+ exit "$failed"

Per https://pagure.io/fesco/issue/2923 enable frame pointers by default now that https://fedoraproject.org/wiki/Changes/fno-omit-frame-pointer has been approved.

Note: this shouldn't be merged until we've coordinated with the Python maintainers.

cc @daandemeyer @churchyard

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

As mentioned in https://src.fedoraproject.org/rpms/redhat-rpm-config/pull-request/230, I think thre should be a CI test to ensure the enabling/disabling works.

@dcavalca Can you fix a formatting error you have at the bottom of the Individual Compiler flags section? It's not part of the PR, but it's about the frame pointers documentation, so...

Here's the broken chunk (note the formatting errors):

If frame pointers are enabled by default (via `%_include_frame_pointers),
the `-fno-omit-frame-pointer` and `-mno-omit-leaf-frame-pointer` flags will
be added.

Surely this doesn't work as submitted because not all architectures support -mno-omit-leaf-frame-pointer?

What's the reason for using -mno-omit-leaf-frame-pointer on aarch64? This architecture uses a link register, so backtracing through leaf functions without a frame pointer does not skip a frame. To me, this suggests that it's possible to keep omitting frame pointers for leaf functions.

-mno-omit-leaf-frame-pointer isn't a valid option for s390x, it needs -mbackchain instead ...

-mno-omit-leaf-frame-pointer isn't a valid option for s390x, it needs -mbackchain instead ...

Isn't this slightly different from -fno-omit-frame-pointer as well? Not sure if we want to use this option.

-mno-omit-leaf-frame-pointer isn't a valid option for s390x, it needs -mbackchain instead ...

Isn't this slightly different from -fno-omit-frame-pointer as well? Not sure if we want to use this option.

I will forward you a reply I got from Uli/IBM, you might be right and it's -mbackchain instead of the both options

What's the reason for using -mno-omit-leaf-frame-pointer on aarch64? This architecture uses a link register, so backtracing through leaf functions without a frame pointer does not skip a frame. To me, this suggests that it's possible to keep omitting frame pointers for leaf functions.

Looking at the kernel unwinder code, for aarch64, it starts with the following (perf_callchain_user() in arch/arm64/kernel/perf_callchain.c):

tail = (struct frame_tail __user *)regs->regs[29];

Register X29 is the frame pointer register on aarch64, so to me this seems like we do need the frame pointer available in leaf functions on aarch64 in order to unwind correctly.

We checked the clang source code and on aarch64, it defaults to omiting the leaf frame pointer even if fno-omit-frame-pointer is specified so we have to add -mno-omit-leaf-frame-pointer to make sure the leaf frame pointer does not get omited. Internally, we use both -fno-omit-leaf-frame-pointer and -mno-omit-leaf-frame-pointer for both x86_64 and aarch64.
Edited 8 months ago by daandemeyer

rebased onto a920e7ec45a4d0529cc66fc55c44708907aa0b73
8 months ago

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.
Warning:
Error merging src.fedoraproject.org/rpms/redhat-rpm-config for 231,92b67c9845ef423f63e0a9431ae4692df58b49df

Updated to use -mbackchain on s390x and to fix and clarify the docs a bit. Still need to add tests, working on that now.

rebased onto 867c1b1ba7d5f9e8f50bc769734e2223a35661d3
8 months ago

Build succeeded.
  • Does %ifarch work in a macro file?
  • What do the added options actually do on arch64 then?

Does %ifarch work in a macro file?

It doesn't , I just noticed now when building the test harness (yay for testing!). Will rework this.

What do the added options actually do on arch64 then?

-fno-omit-frame-pointer is effectively a noop (as the compiler includes frame pointers by default on that arch), -mno-omit-leaf-frame-pointer ensures the leaf frame pointer does not get omitted.

for the record, the ppc64le compiler doesn't accept -mno-omit-leaf-frame-pointer as well

I raised the question on the devel list if the change as voted in is implicitly specific to x86_64:

https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/message/AZBSLQXEHGJLXHR65A5VVZXVF34XNJK4/

Please continue the discussion there.

4 new commits added

  • tests: add test harness for %_include_frame_pointers
  • Do not set -mno-omit-leaf-frame-pointer when including frame pointers on ppc64le
  • Use -mbackchain on s390x when including frame pointers
  • Enable frame pointers by default
8 months ago

Reworked this to not use ifarch, dropped -mno-omit-leaf-frame-pointer from ppc64le and added a test harness.

4 new commits added

  • tests: add test harness for %_include_frame_pointers
  • Do not set -mno-omit-leaf-frame-pointer when including frame pointers on ppc64le
  • Use -mbackchain on s390x when including frame pointers
  • Enable frame pointers by default
8 months ago

Build succeeded.

rebased onto 3621764bcc1ef304d2704900a02a98778b1f33cb
8 months ago

rebased onto f080fb9
8 months ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

3 new commits added

  • tests: add test harness for %_include_frame_pointers
  • Set arch specific flags for frame pointers support
  • Enable frame pointers by default
8 months ago

Build succeeded.

:thumbsup:

If nothing comes up in the meantime, I'll merge this tomorrow afternoon.

You can't use -fno-omit-frame-pointer on i?86, various packages will not build at all in that case.
The problem is mainly 6 argument syscalls (which use ebp as the 6th argument), so nothing that needs to do an inline 6 argument syscall will compile with -fno-omit-frame-pointer, but also due to the shortage of general purpose registers some functions especially with inline asm might not be successfully register allocated. Not to mention that profilers foolishly relying on frame pointers can't work at all if syscall function or 6 argument inline syscall are in the backtrace.

You can't use -fno-omit-frame-pointer on i?86, various packages will not build at all in that case.

Do you have a sense of how many packages would be impacted? In general, I'm inclined to leave this in place and let the mass rebuild flush these out. We can then easily opt them out with something like:

%ifarch %{ix86}
%undefine _include_frame_pointers
%endif

You can't use -fno-omit-frame-pointer on i?86, various packages will not build at all in that case.

I'd like to have those identified and have bug reports for when they opt-out so it can be tracked.

We have a tracker bug for people to link their reports for having to do so too: https://bugzilla.redhat.com/show_bug.cgi?id=IncludeFramePointers

1 new commit added

  • tests: better expose success/failure for include-frame-pointers
8 months ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

1 new commit added

  • tests: symlink macro files into the include-frame-pointers test so they can be found
8 months ago

Build succeeded.

3 new commits added

  • tests: add test harness for %_include_frame_pointers
  • Set arch specific flags for frame pointers support
  • Enable frame pointers by default
8 months ago

Build succeeded.

Tests should all be sorted out at last, apologies for the noise. There are no changes to the actual implementation.

LGTM! Merging.

LGTM! Merging.

Pull-Request has been merged by ngompa
8 months ago

Updated to use -mbackchain on s390x and to fix and clarify the docs a bit. Still need to add tests, working on that now.

Dan just pointed me to that. I notice that you now have -fno-omit-frame-pointer in addition to -mbackchain on s390x. That's not a good idea.

On s390x, the frame pointer does not enable stack backtracing, because it points to the bottom of the stack frame instead of the top (like on x86). As I understand the purpose of this change to be to enable fast runtime stack backtracing, using that option does not help that goal at all on s390x. That's what -mbackchain is for instead.

Use of -fno-omit-frame-pointer, even though useless for the intended purpose, will still cause the overhead of the enforced frame pointer to be incurred (in particular one wasted register), so I would not recommend this.

I've put up https://src.fedoraproject.org/rpms/redhat-rpm-config/pull-request/235 to exclude i686 and s390x for now based on the results of today's FESCo meeting (https://pagure.io/fesco/issue/2923#comment-834996).
Metadata
None
No Tags
Changes Summary 6
+15 -5
file changed
buildflags.md
+7 -4
file changed
macros
+5 -1
file changed
redhat-rpm-config.spec
+11 -11
file changed
rpmrc
+7
file added
tests/include-frame-pointers/main.fmf
+52
file added
tests/include-frame-pointers/runtest.sh
Powered by Pagure 5.13.3
Documentation • File an Issue • About this Instance • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.