920055 - Honor --disable-install-strip and STRIP_FLAGS when packaging compiled tests

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Christian Holler (:decoder)]

In bug 889078, support for stripping the cppunittest binaries was added. However, the binaries are stripped regardless of the --disable-install-strip setting which is supposed to prevent stripping of packaged binaries (which should include test binaries).

Stripped binaries cannot be used to symbolize ASan traces, so it would be good to get unstripped tests when this particular option is set. Especially when the cppunittests are moved into a test job, this would break ASan symbolizing.

Patch coming soon.

Comment 1

[Mike Hommey [:glandium]]

Does ASan need debug info, or just unstripped symbols? In the latter case, then you don't want disable-install-strip, you want to set STRIP_FLAGS.

Comment 2

[Christian Holler (:decoder)]

(In reply to Mike Hommey [:glandium] from comment #1)
> Does ASan need debug info, or just unstripped symbols? In the latter case,
> then you don't want disable-install-strip, you want to set STRIP_FLAGS.

I think it just needs symbols. What kind of strip flags should I try for that purpose? I will try it locally then and if that works, I'll adjust our configurations. 

Even if that works though, we still need a patch here because according to the code, STRIP_FLAGS is also not used when stripping the tests.

Comment 3

[Mike Hommey [:glandium]]

--strip-debug

Comment 4

[Christian Holler (:decoder)]

Okay, I've tried out --strip-debug but with that, we miss the file and line information which is critical. However, --only-keep-debug seems to do what we want. Currently doing a try run with the changes.

Comment 5

[(not currently active) Ted Mielczarek]

I don't think you want --only-keep-debug. If you need file/line info then --strip-debug isn't going to work, you're going to have to stick with unstripped, or fix everything to use Breakpad symbols.

Comment 6

[Christian Holler (:decoder)]

Attachment: strip-flags.patch

Okay, I've written a patch now that just ensures that both the install-strip option and the STRIP_FLAGS variable are honored when packaging the tests. I did not change any of the default options for ASan builds.

In another bug, I might consider building ASan with -gline-tables-only which makes stripping unnecessary, while still reducing the file size dramatically.

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 811038 [details] [diff] [review]
strip-flags.patch

Review of attachment 811038 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/testsuite-targets.mk
@@ +500,5 @@
> +	STRIP_FLAGS=--strip-unneeded
> +endif
> +else
> +	STRIP_FLAGS=
> +endif

I don't think I'd bother with this block. Doesn't seem worth fiddling the STRIP_FLAGS if they're not already set. I'd probably just do
Instead of the above, you could just replace this with
ifdef OBJCOPY
ifndef PKG_SKIP_STRIP
STRIP_CPP_TESTS := 1
endif
endif

Then replace the ifdef OBJCOPY below with ifdef STRIP_CPP_TESTS.

Comment 8

[Christian Holler (:decoder)]

Attachment: strip-flags.patch

Patch v2 with comments addressed.

Comment 9

[Christian Holler (:decoder)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8cf8b27db6ea

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8cf8b27db6ea