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
On Tue, 2025-02-04 at 11:37 +0100, Mathieu Othacehe via lists.openembedded.org wrote: > > 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): One of the reasons this has stayed as pending was this messy argument passing, which I appreciate is pre-existing code. I've sent a patch which cleans it up a bit and this patch will need rebasing on top of it. That should at least make things cleaner. I'm still a bit unsure what to do with the stripping changes this patch makes. One concern I worry about is having "magic" options users need to know to set. The other is whether there is really a binutils type bug underlying this and that there really should be a fix made somewhere else. I don't know enough about the issue to be sure whether there is an underlying issue though... Cheers, Richard
Hello Richard, > One of the reasons this has stayed as pending was this messy argument > passing, which I appreciate is pre-existing code. I've sent a patch > which cleans it up a bit and this patch will need rebasing on top of > it. That should at least make things cleaner. OK, noted. > I'm still a bit unsure what to do with the stripping changes this patch > makes. One concern I worry about is having "magic" options users need > to know to set. The other is whether there is really a binutils type > bug underlying this and that there really should be a fix made > somewhere else. I don't know enough about the issue to be sure whether > there is an underlying issue though... I think that this can be seen in a more generic way. To get a backtrace, libunwind supports multiple methods on ARM: --8<---------------cut here---------------start------------->8--- /* unwinding method selection support */ #define UNW_ARM_METHOD_ALL 0xFF #define UNW_ARM_METHOD_DWARF 0x01 #define UNW_ARM_METHOD_FRAME 0x02 #define UNW_ARM_METHOD_EXIDX 0x04 #define UNW_ARM_METHOD_LR 0x08 --8<---------------cut here---------------end--------------->8--- The default one UNW_ARM_METHOD_EXIDX only supports partial backtraces, as explained in the GCC thread[1]. Then, the UNW_ARM_METHOD_DWARF method that provides complete backtraces operates on the ELF .debug_frame. By always stripping that section, and offering no way of keeping this section, we are preventing the user to use that unwinding method altogether. That is true for ARM but for other architectures as well. It can very be that one might want to keep the .debug_frame around on x86_64 so that the same unwinding method is used across all the architectures supported for a given image. Maybe going for the v2 approach allowing to keep any ELF section, would feel less magical than the v1 approach that was targeting specifically the .debug_frame section. Finally, the .debug_frame based unwinding was a bit broken on libunwind, it this now fixed[2], so it would be great to be able to use it in Yocto. Thanks, Mathieu [1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117941 [2]: https://github.com/libunwind/libunwind/pull/826
On Fri, 2025-02-28 at 10:09 +0100, Mathieu Othacehe wrote: > > Hello Richard, > > > One of the reasons this has stayed as pending was this messy argument > > passing, which I appreciate is pre-existing code. I've sent a patch > > which cleans it up a bit and this patch will need rebasing on top of > > it. That should at least make things cleaner. > > OK, noted. > > > I'm still a bit unsure what to do with the stripping changes this patch > > makes. One concern I worry about is having "magic" options users need > > to know to set. The other is whether there is really a binutils type > > bug underlying this and that there really should be a fix made > > somewhere else. I don't know enough about the issue to be sure whether > > there is an underlying issue though... > > I think that this can be seen in a more generic way. To get a backtrace, > libunwind supports multiple methods on ARM: > > --8<---------------cut here---------------start------------->8--- > /* unwinding method selection support */ > #define UNW_ARM_METHOD_ALL 0xFF > #define UNW_ARM_METHOD_DWARF 0x01 > #define UNW_ARM_METHOD_FRAME 0x02 > #define UNW_ARM_METHOD_EXIDX 0x04 > #define UNW_ARM_METHOD_LR 0x08 > --8<---------------cut here---------------end--------------->8--- > > The default one UNW_ARM_METHOD_EXIDX only supports partial backtraces, > as explained in the GCC thread[1]. Then, the UNW_ARM_METHOD_DWARF method > that provides complete backtraces operates on the ELF .debug_frame. By > always stripping that section, and offering no way of keeping this > section, we are preventing the user to use that unwinding method > altogether. > > That is true for ARM but for other architectures as well. It can very be > that one might want to keep the .debug_frame around on x86_64 so that > the same unwinding method is used across all the architectures supported > for a given image. > > Maybe going for the v2 approach allowing to keep any ELF section, would > feel less magical than the v1 approach that was targeting specifically > the .debug_frame section. > > Finally, the .debug_frame based unwinding was a bit broken on libunwind, > it this now fixed[2], so it would be great to be able to use it in > Yocto. Thanks for the info, this helps but I guess I still have a question. You're using this with minidebuginfo so does it not make sense to add this as one of the debug sections injected there with the rest of the minidebug info? The option users would would then see would be whether to include frame information in the minidebuginfo section and we'd benefit from compression. Whether the tools support accessing it from such a section, I don't know. We may also need to make the sections preserved architecture dependent as I think good unwinding varies by architecture based on what you said previously. My main concern here is how to expose this in a way which users can understand and use. Cheers, Richard
Hello Richard, > You're using this with minidebuginfo so does it not make sense to add > this as one of the debug sections injected there with the rest of the > minidebug info? Well minidebuginfo is about having compressed symbols in an ELF section, and keeping .debug_frame is about having unwinding material in another ELF section. In the end both of them are needed to get human-readable backtraces on target. Now, keeping .debug_frame when minidebuginfo is enabled seems a bit odd. We could only keep .debug_frame on the needed architectures (ARMv7) and rely on the more generic .eh_frame sections that are never stripped on other architectures (aarch64, x86_64). Yet, picking what to strip based on whether minidebuginfo is enabled does not feel right. Maybe, we could go for a DISTRO_FEATURE that would be even more generic, such as "backtrace_on_target" that would enable minidebuginfo and keep the suitable unwinding sections based on the architecture. That way, users would have unwinding materials and symbols so that GDB, libunwind, systemd-coredump and other backtracing tools always work on target with a minimal overhead. People could enable "backtrace_on_target", see if the image overhead is acceptable to them, and have backtraces without diving into complex details. I think it would still be needed to offer minidebuginfo and PACKAGE_KEEP_SECTIONS so that users willing to invest more time can decide precisely what do they want on the target in terms of symbols and unwinding material. What would you think about that? Thanks, Mathieu
On Sat, 2025-03-01 at 10:31 +0100, Mathieu Othacehe wrote: > > Hello Richard, > > > You're using this with minidebuginfo so does it not make sense to add > > this as one of the debug sections injected there with the rest of the > > minidebug info? > > Well minidebuginfo is about having compressed symbols in an ELF section, > and keeping .debug_frame is about having unwinding material in another > ELF section. In the end both of them are needed to get human-readable > backtraces on target. I think the name has been misleading me as I imagined minidebuginfo as a small subset of the main debug information rather than just the symbols. You're saying it is just the symbols and there is no way to extend the compressed information? > Now, keeping .debug_frame when minidebuginfo is enabled seems a bit > odd. We could only keep .debug_frame on the needed architectures (ARMv7) > and rely on the more generic .eh_frame sections that are never stripped > on other architectures (aarch64, x86_64). Yet, picking what to strip > based on whether minidebuginfo is enabled does not feel right. My point was more that if you're enabling minidebuginfo, you are probably going to want good unwinding information. I'm struggling to see users enabling one without really wanting the other. > Maybe, we could go for a DISTRO_FEATURE that would be even more generic, > such as "backtrace_on_target" that would enable minidebuginfo and keep > the suitable unwinding sections based on the architecture. That way, > users would have unwinding materials and symbols so that GDB, libunwind, > systemd-coredump and other backtracing tools always work on target with > a minimal overhead. People could enable "backtrace_on_target", see if > the image overhead is acceptable to them, and have backtraces without > diving into complex details. > > I think it would still be needed to offer minidebuginfo and > PACKAGE_KEEP_SECTIONS so that users willing to invest more time can > decide precisely what do they want on the target in terms of symbols and > unwinding material. > > What would you think about that? OE is about choice but I'm still thinking that if someone enables minidebuginfo, they are most likely expecting stack traces to work as otherwise the extra symbol information isn't much use. Am is missing some key usage? If PACKAGE_KEEP_SECTIONS worked well for all targets, I think I'd be happier to consider it but the problem is that it is all very architecture dependent :( Cheers, Richard
On Sat, Mar 1, 2025 at 1:32 AM Mathieu Othacehe via lists.openembedded.org <othacehe=gnu.org@lists.openembedded.org> wrote: > > Hello Richard, > > > You're using this with minidebuginfo so does it not make sense to add > > this as one of the debug sections injected there with the rest of the > > minidebug info? > > Well minidebuginfo is about having compressed symbols in an ELF section, > and keeping .debug_frame is about having unwinding material in another > ELF section. In the end both of them are needed to get human-readable > backtraces on target. > > Now, keeping .debug_frame when minidebuginfo is enabled seems a bit > odd. We could only keep .debug_frame on the needed architectures (ARMv7) > and rely on the more generic .eh_frame sections that are never stripped > on other architectures (aarch64, x86_64). Yet, picking what to strip > based on whether minidebuginfo is enabled does not feel right. > ARM EHABI defines two new sections exidx and extab for storing exception indexes and exception unwind data which is what other architecture use .eh_frame for at runtime. The rationale to have it done this way is reduced size. Can minidebuginfo use these sections in same way like it uses .eh_frame ? > Maybe, we could go for a DISTRO_FEATURE that would be even more generic, > such as "backtrace_on_target" that would enable minidebuginfo and keep > the suitable unwinding sections based on the architecture. That way, > users would have unwinding materials and symbols so that GDB, libunwind, > systemd-coredump and other backtracing tools always work on target with > a minimal overhead. People could enable "backtrace_on_target", see if > the image overhead is acceptable to them, and have backtraces without > diving into complex details. > > I think it would still be needed to offer minidebuginfo and > PACKAGE_KEEP_SECTIONS so that users willing to invest more time can > decide precisely what do they want on the target in terms of symbols and > unwinding material. > > What would you think about that? > > Thanks, > > Mathieu > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#212091): > https://lists.openembedded.org/g/openembedded-core/message/212091 > Mute This Topic: https://lists.openembedded.org/mt/110989612/1997914 > Group Owner: openembedded-core+owner@lists.openembedded.org > Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [ > raj.khem@gmail.com] > -=-=-=-=-=-=-=-=-=-=-=- > >
Hello Richard, > I think the name has been misleading me as I imagined minidebuginfo as > a small subset of the main debug information rather than just the > symbols. You're saying it is just the symbols and there is no way to > extend the compressed information? The minidebuginfo symbols are stored in: --8<---------------cut here---------------start------------->8--- subprocess.check_call([objcopy, '--add-section', '.gnu_debugdata={}.xz'.format(minidebugfile), file]) --8<---------------cut here---------------end--------------->8--- the .gnu_debugdata ELF section. That's a convention and some tools out there (GDB, libunwind) will search for symbols in that specific section. > OE is about choice but I'm still thinking that if someone enables > minidebuginfo, they are most likely expecting stack traces to work as > otherwise the extra symbol information isn't much use. Am is missing > some key usage? Another usage would be to debug a live program on target with GDB. You need the symbols but not necessarily the unwind information to do that I guess. > If PACKAGE_KEEP_SECTIONS worked well for all targets, I think I'd be > happier to consider it but the problem is that it is all very > architecture dependent :( Would it be OK if I send a v3 that re-introduces PACKAGE_KEEP_DEBUG_FRAME, and enables that one if minidebuginfo is enabled and the target architecture is ARM? Thanks, Mathieu
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(-)