Re: [Buildroot] [PATCH 4/6] support/testing/tests/package/test_google_breakpad.py: new runtime test

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

Hello,

Julien (Olivain, there is some question for you towards the end).

On Sat,  2 Dec 2023 21:18:37 -0700
Adam Duskett <adam.duskett@amarulasolutions.com> wrote:

> Add a test that does the following:
> 
>   - Builds the example code found here:
>     https://chromium.googlesource.com/breakpad/breakpad/+/master/docs/linux_starter_guide.md
> 
>   - runs the google-breakpad-example program and checks to ensure the exit
>     code is 139 (SIGSEGV)
> 
>   - Runs the minidump_stackwalk tool against the google-breakpad-example
>     symbols and checks for the following:
>     - That the google-breakpad-examples symbols loaded properly.
>     - That the crash() method in the google-breakpad-example.cpp file is
>       printed.
> 
> Signed-off-by: Adam Duskett <adam.duskett@amarulasolutions.com>

Very nice test case! Clearly not the simplest test case. I have some
suggestions below.


> diff --git a/support/testing/tests/package/br2-external/google-breakpad/package/google-breakpad-example/google-breakpad-example.mk b/support/testing/tests/package/br2-external/google-breakpad/package/google-breakpad-example/google-breakpad-example.mk
> new file mode 100644
> index 0000000000..17ddd68716
> --- /dev/null
> +++ b/support/testing/tests/package/br2-external/google-breakpad/package/google-breakpad-example/google-breakpad-example.mk
> @@ -0,0 +1,24 @@
> +################################################################################
> +#
> +# google-breakpad-example
> +#
> +################################################################################
> +
> +GOOGLE_BREAKPAD_EXAMPLE_DEPENDENCIES = google-breakpad
> +
> +define GOOGLE_BREAKPAD_EXAMPLE_BUILD_CMDS
> +	$(INSTALL) -D $(GOOGLE_BREAKPAD_EXAMPLE_PKGDIR)/google-breakpad-example.cpp \
> +		$(@D)/google-breakpad-example.cpp

Trivial: I think this particular command should go into
GOOGLE_BREAKPAD_EXAMPLE_EXTRACT_CMDS.

> +
> +	$(TARGET_MAKE_ENV) $(TARGET_CXX) $(TARGET_CPPFLAGS) \
> +		-I$(STAGING_DIR)/usr/include/breakpad \
> +		$(@D)/google-breakpad-example.cpp \
> +		$(STAGING_DIR)/usr/lib/libbreakpad_client.a -o $(@D)/google-breakpad-example

Perhaps you can use -lbreakpad_client instead of
$(STAGING_DIR)/usr/lib/libbreakpad_client.a ?

> diff --git a/support/testing/tests/package/br2-external/google-breakpad/post-build.sh b/support/testing/tests/package/br2-external/google-breakpad/post-build.sh
> new file mode 100755
> index 0000000000..6778aae339
> --- /dev/null
> +++ b/support/testing/tests/package/br2-external/google-breakpad/post-build.sh
> @@ -0,0 +1,5 @@
> +#!/usr/bin/env bash
> +set -e
> +
> +# We are going to run the minidump_stackwalk tool in the qemu instance.
> +cp -rf "${STAGING_DIR}"/usr/share/google-breakpad-symbols "${TARGET_DIR}"/root/

See below, I have a suggestion that I believe would make this
post-build script not necessary.

> diff --git a/support/testing/tests/package/test_google_breakpad.py b/support/testing/tests/package/test_google_breakpad.py
> new file mode 100644
> index 0000000000..646bdf6ee3
> --- /dev/null
> +++ b/support/testing/tests/package/test_google_breakpad.py
> @@ -0,0 +1,50 @@
> +import os
> +import infra.basetest
> +
> +
> +class GoogelBreakPadBase(infra.basetest.BRTest):

            ^^^ minor typo here

> +    br2_external = [infra.filepath("tests/package/br2-external/google-breakpad")]
> +    config = \
> +        """
> +        BR2_arm=y
> +        BR2_TOOLCHAIN_EXTERNAL=y
> +        BR2_TOOLCHAIN_EXTERNAL_BOOTLIN=y
> +        BR2_ENABLE_DEBUG=y
> +        BR2_ROOTFS_POST_BUILD_SCRIPT="support/testing/tests/package/br2-external/google-breakpad/post-build.sh"
> +        BR2_STRIP_EXCLUDE_FILES="google-breakpad-example"

So the target exclude needs to have debugging symbols for
google-breakpad to work? I thought the whole idea was precisely that it
allowed to avoid the need for having debugging symbols on the target? I
must admit I am not very familiar with google-breakpad, so I might
totally wrong on this.

> +        BR2_GOOGLE_BREAKPAD_ENABLE=y
> +        BR2_GOOGLE_BREAKPAD_INCLUDE_FILES="/usr/bin/google-breakpad-example"
> +        BR2_TARGET_ROOTFS_CPIO=y
> +        BR2_PACKAGE_GOOGLE_BREAKPAD=y
> +        BR2_PACKAGE_GOOGLE_BREAKPAD_TOOLS=y

With my proposal below, the tools on the target would not be needed.

> +        BR2_PACKAGE_GOOGLE_BREAKPAD_EXAMPLE=y
> +        """
> +
> +    def check_google_breakpad(self):
> +        cpio_file = os.path.join(self.builddir, "images", "rootfs.cpio")
> +        self.emulator.boot(arch="armv5",
> +                           kernel="builtin",
> +                           options=["-initrd", cpio_file])
> +        self.emulator.login()
> +        output, exit_code = self.emulator.run("google-breakpad-example")
> +
> +        # google-breakpad-example is expected to exit SIGSEGV.
> +        self.assertEqual(exit_code, 139)
> +        dump_path = output[0].split(' ')[2]
> +        output, exit_code = self.emulator.run(f"minidump_stackwalk {dump_path} /root/google-breakpad-symbols/")
> +        self.assertEqual(exit_code, 0)
> +
> +        # Ensure that minidump_stackwalk properly loaded the google-breakpad-example symbols
> +        if not any("Loading symbols for module /usr/bin/google-breakpad-example" in line for line in output):
> +            self.fail("Failed to load symbols from /usr/bin/google-breakpad-example")
> +
> +        # Ensure that the crash method is found and printed by minidump_stackwalk
> +        if not any("google-breakpad-example!crash()" in line for line in output):
> +            self.fail("google-breakpad-example!crash() not in minidump_stackwalk output!")

I think the analysis of the dump should happen on the host rather than
the target, because this is really the primary use-case for
google-breakpad: your target application is linked with
google-breakpad, so that when it crashes it creates a minidump, which
you can then retrieve for off-line analysis with the debugging symbols
that you have kept around in your build system.

So after the application is run and has crashed, I believe we should
transfer the crash data back to the host, and then use
minidump_stackwalk on the host.

As we discussed, I am just not sure what would be the right mechanism
to exchange data between the target system running in Qemu, and the
host running the test suite. Our default Busybox configuration doesn't
enable httpd or ftpd. One option would be dropbear of course. This
would in all cases require networking between host and target.

Julien: do you have some suggestions here?

> +class TestGoogleBreakPadGlibC(GoogelBreakPadBase):
> +    config = GoogelBreakPadBase.config + "BR2_TOOLCHAIN_EXTERNAL_BOOTLIN_ARMV5_EABI_GLIBC_STABLE=y"
> +
> +    def test_run(self):
> +        self.check_google_breakpad()

Since for now we're keeping breakpad glibc-only, let's keep things
simple and have the main test case use a glibc toolchain. We can always
rework this if uclibc (or musl) support gets enabled at some point in
the future.

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot