1595906 - Allow opting out of hardening compile flags on a granular basis

Status: RESOLVED FIXED

(Firefox Build System :: General, task)

Description

[Ricky Stewart]

The hardening compile flags are baked-in everywhere and they unconditionally break wasm compilation (since they generate asm instructions that wasm considers unsafe). Allow opting out of including them in CFLAGS, CXXFLAGS, etc.

Comment 1

[Ricky Stewart]

Attachment: Bug 1595906 - Allow opting out of hardening compile flags on a granular basis

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Ricky Stewart from comment #0)

The hardening compile flags are baked-in everywhere and they unconditionally break wasm compilation (since they generate asm instructions that wasm considers unsafe). Allow opting out of including them in CFLAGS, CXXFLAGS, etc.

Should we even be using CFLAGS etc. for wasm, since it is effectively a different target, just like the host would be in a cross-compilation scenario?

Comment 3

[(Away)]

The title of the patch confused me for a while, I wanted to say "well that's what --disable-hardening is for".

I think I eventually figured out, you want to opt-out of these flags at the individual moz.build level, is that right? If so, it might help to clarify that.

Comment 4

[Ricky Stewart]

(In reply to Nathan Froyd [:froydnj] from comment #2)

(In reply to Ricky Stewart from comment #0)

The hardening compile flags are baked-in everywhere and they unconditionally break wasm compilation (since they generate asm instructions that wasm considers unsafe). Allow opting out of including them in CFLAGS, CXXFLAGS, etc.

Should we even be using CFLAGS etc. for wasm, since it is effectively a different target, just like the host would be in a cross-compilation scenario?

I considered this and am open to discussion about it. What I figured was that adding a new set of variables (WASM_CFLAGS, WASM_CXXFLAGS, WASM_LDFLAGS) would be relatively high-friction and require a lot of new code including duplicate or near-duplicate features across CFLAGS/CXXFLAGS/LDFLAGS/etc. and their WASM_ counterparts. Moreover I thought this would have a relatively low return on investment since if we're compiling the same library for Linux and for (Linux through sandboxed WASM through Lucet), the two libraries would probably be compiled in almost exactly the same configurations (the same CFLAGS, CXXFLAGS, etc., except for those flags that are incompatible with WASM), so not using CFLAGS, CXXFLAGS, etc. would just unnecessarily limit opportunities for code reuse.

I'm not necessarily opposed to going the other way on this but I hope we're all in agreement that the investment in the extra work makes sense.

Adding some folks to CC for FYI/additional discussion.

Comment 5

[Chris Manchester (limited bugmail, email directly)]

I don't think we're going to know for sure until we do the work of figuring out exactly what flags we want and where they might differ -- "the same CFLAGS, CXXFLAGS, etc., except for those flags that are incompatible with WASM" makes me a little nervous. Initially though, I'd be surprised if we could re-use these verbatim. We also may hit issues if we're ever going to want to compile to wasm and do a regular compile in the same directory.

The way I might expect this to work would be to have our WASM_CFLAGS etc. set based off of the shareable subset of CFLAGS the way we share some things like DEFINES and INCLUDES between AsmFlags, HostCompileFlags, and CompileFlags in context.py today.

Comment 6

[Ricky Stewart]

Ack -- I'll continue to land revision D52756 and close this bug out just for the sake of getting this logic out of old-configure and into Python, but I'll open another one for adding WASM_CFLAGS, etc

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/868a55980ae8
Allow opting out of hardening compile flags on a granular basis r=froydnj

Comment 8

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/868a55980ae8

Comment 9

[Florin Strugariu [:Bebe]]

== Change summary for alert #23970 (as of Fri, 15 Nov 2019 10:38:26 GMT) ==

Improvements:

3% ts_paint windows7-32-shippable opt e10s stylo 319.42 -> 311.17
3% ts_paint windows7-32-shippable opt e10s stylo 319.17 -> 311.00
2% ts_paint_webext windows7-32-shippable opt e10s stylo 318.92 -> 311.83

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=23970

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Florin Strugariu [:Bebe] (needinfo me) from comment #9)

== Change summary for alert #23970 (as of Fri, 15 Nov 2019 10:38:26 GMT) ==

Improvements:

3% ts_paint windows7-32-shippable opt e10s stylo 319.42 -> 311.17
3% ts_paint windows7-32-shippable opt e10s stylo 319.17 -> 311.00
2% ts_paint_webext windows7-32-shippable opt e10s stylo 318.92 -> 311.83

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=23970

Assuming that this is actually attributable to these changes, this is a little worrisome, because these changes shouldn't have had any impact.

The graph says the push responsible is:

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=512d489d83e128358c960823d75b22bd59cb7163&tochange=24b134b55c5f70eab758d74682fc1f6ca829e0ff

which has some other stuff mixed in, but nothing that I would expect to affect win32 specifically and solely. Ricky, can you double-check the win32 logs to make sure we're still applying the same options before and after your push?

Comment 11

[Ricky Stewart]

I peeked at the build logs and config.status for the push in questionand nothing looks amiss; we're still passing in the same hardening flags (namely, "-guard:cf"). I think everything looks fine, but I'll also follow up with one of my other build buddies to make sure I'm not missing anything.

Comment 12

[(Away)]

I added g5 jobs to each individual push in that range, and (ignoring a couple of outliers) it certainly looks like the change began with this bug: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&searchStr=windows7%2Cship%2Cg5&tochange=24b134b55c5f70eab758d74682fc1f6ca829e0ff&fromchange=512d489d83e128358c960823d75b22bd59cb7163

Comment 13

[(Away)]

Something's definitely up:

$ diff obj-before/toolkit/crashreporter/backend.mk obj-after/toolkit/crashreporter/backend.mk
33c33,39
< COMPUTED_LDFLAGS += -guard:cf,nolongjmp -LARGEADDRESSAWARE -SAFESEH -DEBUG -OPT:REF,ICF
---
> COMPUTED_A += -guard:cf,nolongjmp
> COMPUTED_D += -guard:cf,nolongjmp
> COMPUTED_G += -guard:cf,nolongjmp
> COMPUTED_F += -guard:cf,nolongjmp
> COMPUTED_L += -guard:cf,nolongjmp -guard:cf,nolongjmp
> COMPUTED_S += -guard:cf,nolongjmp
> COMPUTED_LDFLAGS += -LARGEADDRESSAWARE -SAFESEH -DEBUG -OPT:REF,ICF

And -guard:cf is also missing from COMPUTED_CXXFLAGS etc. (won't paste the diff since it's huge)

But I'll step out of the way and let Ricky continue from here. :-)

Comment 14

[Ricky Stewart]

Oh, what fun, this is CERTAINLY a Python dynamic-typing issue. A string ended up somewhere where a list of strings should have been.

Hopefully this is a simple change and I can fix forward with a 1-line change rather than rolling this back.

Comment 15

[(Away)]

And -guard:cf is also missing from COMPUTED_CXXFLAGS etc. (won't paste the diff since it's huge)

On closer inspection I don't think there was an actual regression with COMPUTED_CXXFLAGS, this bug's patch deduplicated -guard:cf so I noticed one missing when there were previously two.

Anyway, what matters is that the linked bug addresses the issue with COMPUTED_LDFLAGS.