Closed Bug 920055 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox Build System :: General, defect, major)

x86_64
Linux
defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED
mozilla27

People

(Reporter: decoder, Assigned: decoder)

References

(Blocks 1 open bug)

Details

(Keywords: sec-want)

Attachments

(1 file, 1 obsolete file)

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.
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.
(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.
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.
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.
Summary: Don't strip cppunittests when --disable-install-strip is set → Honor --disable-install-strip and STRIP_FLAGS when packaging compiled tests
Attached patch strip-flags.patch (obsolete) — Splinter Review
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.
Assignee: nobody → choller
Status: NEW → ASSIGNED
Attachment #811038 - Flags: review?(ted)
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.
Attachment #811038 - Flags: review?(ted)
Patch v2 with comments addressed.
Attachment #811038 - Attachment is obsolete: true
Attachment #811089 - Flags: review?(ted)
Attachment #811089 - Flags: review?(ted) → review+
https://hg.mozilla.org/mozilla-central/rev/8cf8b27db6ea
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.