| Message ID | 20250204103744.27883-1-othacehe@gnu.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] lib/oe/package: Add strip keep-section support | expand |
On Tue, 4 Feb 2025 at 11:37, Mathieu Othacehe via lists.openembedded.org <othacehe=gnu.org@lists.openembedded.org> wrote: > Add a 'PACKAGE_KEEP_SECTIONS' variable to keep some specific ELF sections > while stripping binaries and libraries. > > That one can then be used to keep the .debug_frame section around for > example, this way: > > PACKAGE_KEEP_SECTIONS = ".debug_frame" > > By using libunwind + minidebuginfo, that provides a way for users to get > debug_frame based backtraces on target. I don't understand. We already have support and tests for minidebuginfo, so what additional functionality would the above setting enable? Can you adjust the existing tests to showcase that? Adding new variables has a high bar for acceptance, because the need for them must be clearly shown, and they need to be tested and documented too. Also the patch is changing things all over the place, and so is difficult to review. Can you split it up? E.g. first add some new thing, then adjust code elsewhere to take it into use. Alex
On Tue, 4 Feb 2025 at 12:37, Alexander Kanavin <alex.kanavin@gmail.com> wrote: > I don't understand. We already have support and tests for > minidebuginfo, so what additional functionality would the above > setting enable? Can you adjust the existing tests to showcase that? Specifically, this: https://git.yoctoproject.org/poky/tree/meta/lib/oeqa/selftest/cases/minidebuginfo.py Alex
Hello Alexander, > I don't understand. We already have support and tests for > minidebuginfo, so what additional functionality would the above > setting enable? Can you adjust the existing tests to showcase that? When you want to display a backtrace on target you need basically two things: 1. Symbols 2. Unwind information The minidebuginfo functionality is covering the first part. It is providing some symbols in a compressed way, that get appended to every binary/library at build time. Then the second part is about how to unwind the stack. That is architecture dependant. On ARMv7, GCC is producing unwind information in the .debug_frame section. That section is always removed by 'strip'. It prevents any unwinding based on .debug_frame on the target. In the first version of that patch[1], I was proposing to introduce a variable called PACKAGE_KEEP_DEBUG_FRAME to instruct `strip` to keep the .debug_frame section around. Quentin, pointed out, that we could be more generic and offer a PACKAGE_KEEP_SECTIONS variable instead, to prevent specific sections from being stripped. That's what is done in this v2. I have proposed a documentation update here: https://lists.yoctoproject.org/g/docs/message/6243 > Also the patch is changing things all over the place, and so is > difficult to review. Can you split it up? E.g. first add some new > thing, then adjust code elsewhere to take it into use. It is only modifying the run_strip function and its callers, so "all over the place" seems like an overstatement. I am not really sure how to break that into more pieces. It's just about instructing "strip" to keep some sections that are specified in the PACKAGE_KEEP_SECTIONS variable. I would be glad to bring some more details on the topic if needed. Thanks, Mathieu
On Tue, 4 Feb 2025 at 13:36, Mathieu Othacehe <othacehe@gnu.org> wrote: > Quentin, pointed out, that we could be more generic and offer a > PACKAGE_KEEP_SECTIONS variable instead, to prevent specific sections > from being stripped. > > That's what is done in this v2. I have proposed a documentation update > here: https://lists.yoctoproject.org/g/docs/message/6243 Thanks. But you still need to look at existing minidebuginfo tests and see if you can enhance them. Otherwise we have a variable that isn't tested. Also, if minidebuginfo distro feature is enabled, do we actually need this level of control? Why not just always have the needed bits? > It is only modifying the run_strip function and its callers, so "all over > the place" seems like an overstatement. I am not really sure how to > break that into more pieces. It's just about instructing "strip" to keep > some sections that are specified in the PACKAGE_KEEP_SECTIONS variable. You can split into at least four commits: - change of runstrip signature - change of strip_execs signature - adding new variable to package.py - adding new variable to staging class. Alex
> Thanks. But you still need to look at existing minidebuginfo tests and > see if you can enhance them. Otherwise we have a variable that isn't > tested. OK, I will check want I can do. > Also, if minidebuginfo distro feature is enabled, do we actually need > this level of control? Why not just always have the needed bits? On some architectures, such as x86_64 and aarch64, the .eh_frame section that is *not* stripped because used for C++ exception handling, can be used for DWARF-based stack unwinding purposes. On those architectures, one can rely on .eh_frame + minidebuginfo to get DWARF based backtraces. So no need for the PACKAGE_KEEP_SECTIONS there. However, on ARMv7, the .eh_frame section is empty and the ARM specific .exidx section is not enough to get full backtraces, as discussed here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117941 For that specific architecture, it then makes sense to instruct 'strip' to leave the .debug_frame section. By setting for instance: PACKAGE_KEEP_SECTIONS:armv7a = ".debug_frame" So minidebuginfo and PACKAGE_KEEP_SECTIONS are two very different things, serving different purposes. Even though the combination of both is required on some architectures to get full backtraces, I don't think that they should be tied together. > You can split into at least four commits: > > - change of runstrip signature > - change of strip_execs signature > - adding new variable to package.py > - adding new variable to staging class. OK, thanks for the suggestion. Mathieu
On Tue, 4 Feb 2025 at 15:25, Mathieu Othacehe <othacehe@gnu.org> wrote: > So minidebuginfo and PACKAGE_KEEP_SECTIONS are two very different > things, serving different purposes. Even though the combination of both > is required on some architectures to get full backtraces, I don't think > that they should be tied together. Fair enough. What you say should be documented though, perhaps you could revise your docs patch to include these things. If you figure out how to test, that would be good as well. Ideally without adding another image build to the test, but adjusting/extending something that already exists. Alex
diff --git a/meta/classes-global/staging.bbclass b/meta/classes-global/staging.bbclass index 1008867a6c..7083878c73 100644 --- a/meta/classes-global/staging.bbclass +++ b/meta/classes-global/staging.bbclass @@ -91,10 +91,12 @@ python sysroot_strip () { base_libdir = d.getVar("base_libdir") qa_already_stripped = 'already-stripped' in (d.getVar('INSANE_SKIP:' + pn) or "").split() strip_cmd = d.getVar("STRIP") + keep_sections = d.getVar('PACKAGE_KEEP_SECTIONS') or "" max_process = oe.utils.get_bb_number_threads(d) oe.package.strip_execs(pn, dstdir, strip_cmd, libdir, base_libdir, max_process, - qa_already_stripped=qa_already_stripped) + qa_already_stripped=qa_already_stripped, + keep_sections=keep_sections) } do_populate_sysroot[dirs] = "${SYSROOT_DESTDIR}" diff --git a/meta/classes-recipe/kernel.bbclass b/meta/classes-recipe/kernel.bbclass index 617727a989..d3d3d9fa65 100644 --- a/meta/classes-recipe/kernel.bbclass +++ b/meta/classes-recipe/kernel.bbclass @@ -770,7 +770,7 @@ python do_strip() { if (extra_sections and kernel_image.find(d.getVar('KERNEL_IMAGEDEST') + '/vmlinux') != -1): kernel_image_stripped = kernel_image + ".stripped" shutil.copy2(kernel_image, kernel_image_stripped) - oe.package.runstrip((kernel_image_stripped, 8, strip, extra_sections)) + oe.package.runstrip((kernel_image_stripped, 8, strip), '', extra_sections) bb.debug(1, "KERNEL_IMAGE_STRIP_EXTRA_SECTIONS is set, stripping sections: " + \ extra_sections) } diff --git a/meta/lib/oe/package.py b/meta/lib/oe/package.py index 1af10b7eb0..468f331fce 100644 --- a/meta/lib/oe/package.py +++ b/meta/lib/oe/package.py @@ -18,7 +18,7 @@ import shutil import oe.cachedpath -def runstrip(arg): +def runstrip(arg, keep_sections='', extra_strip_sections=''): # Function to strip a single file, called from split_and_strip_files below # A working 'file' (one which works on the target architecture) # @@ -27,12 +27,7 @@ def runstrip(arg): # 4 - executable # 8 - shared library # 16 - kernel module - - if len(arg) == 3: - (file, elftype, strip) = arg - extra_strip_sections = '' - else: - (file, elftype, strip, extra_strip_sections) = arg + (file, elftype, strip) = arg newmode = None if not os.access(file, os.W_OK) or os.access(file, os.R_OK): @@ -60,6 +55,10 @@ def runstrip(arg): for section in extra_strip_sections.split(): stripcmd.extend(["--remove-section=" + section]) + if keep_sections != '' and not elftype & 16: + for section in keep_sections.split(): + stripcmd.extend(["--keep-section=" + section]) + stripcmd.append(file) bb.debug(1, "runstrip: %s" % stripcmd) @@ -115,7 +114,8 @@ def is_static_lib(path): return start == magic return False -def strip_execs(pn, dstdir, strip_cmd, libdir, base_libdir, max_process, qa_already_stripped=False): +def strip_execs(pn, dstdir, strip_cmd, libdir, base_libdir, max_process, qa_already_stripped=False, + keep_sections=''): """ Strip executable code (like executables, shared libraries) _in_place_ - Based on sysroot_strip in staging.bbclass @@ -194,7 +194,8 @@ def strip_execs(pn, dstdir, strip_cmd, libdir, base_libdir, max_process, qa_alre elf_file = int(elffiles[file]) sfiles.append((file, elf_file, strip_cmd)) - oe.utils.multiprocess_launch_mp(runstrip, sfiles, max_process) + oe.utils.multiprocess_launch_mp(runstrip, sfiles, max_process, + extraargs=(keep_sections, '')) TRANSLATE = ( ("@", "@at@"), @@ -1305,7 +1306,9 @@ def process_split_and_strip_files(d): for f in staticlibs: sfiles.append((f, 16, strip)) - oe.utils.multiprocess_launch(oe.package.runstrip, sfiles, d) + keep_sections = d.getVar('PACKAGE_KEEP_SECTIONS') or "" + oe.utils.multiprocess_launch(oe.package.runstrip, sfiles, d, + extraargs=(keep_sections, '')) # Build "minidebuginfo" and reinject it back into the stripped binaries if bb.utils.contains('DISTRO_FEATURES', 'minidebuginfo', True, False, d):
Add a 'PACKAGE_KEEP_SECTIONS' variable to keep some specific ELF sections while stripping binaries and libraries. That one can then be used to keep the .debug_frame section around for example, this way: PACKAGE_KEEP_SECTIONS = ".debug_frame" By using libunwind + minidebuginfo, that provides a way for users to get debug_frame based backtraces on target. Signed-off-by: Mathieu Othacehe <othacehe@gnu.org> --- v1: https://lists.openembedded.org/g/openembedded-core/message/209545 documentation: https://lists.yoctoproject.org/g/docs/message/6243 meta/classes-global/staging.bbclass | 4 +++- meta/classes-recipe/kernel.bbclass | 2 +- meta/lib/oe/package.py | 23 +++++++++++++---------- 3 files changed, 17 insertions(+), 12 deletions(-)