853494 - both --disable-optimize and --enable-debug should imply --disable-icf

Status: RESOLVED WONTFIX

(Firefox Build System :: General, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

People use --disable-optimize to get debuggable builds.

--disable-icf is very important in this respect because without it, breakpoints are garbled.

Please make --disable-optimize imply --disable-icf.

Comment 1

[Bas Schouten (:bas.schouten)]

This is one of the most frustrating aspects of debugging B2G. It's already cost me a lot of time as you find yourself wondering what you're doing in <insert random class here>::<insert random function here> when you haven't even instantiated <said random class> and waste a lot of time only to find out ICF messed something up.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

That also means that --disable-icf should be disabled not just by --disable-optimize, but also by something like --enable-debug. The idea is that on B2G, practically nobody is patient enough to use --disable-optimize (people only pass -O0 in specific dirs).

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

I tried to fix this, but I'm running into an issue: the configure code that checks for --disable-icf and runs the test to verify that the linker supports the corresponding LDFLAGS, is not coming from configure.in.

So I don't see a way to implement "let --disable-optimize and --enable-debug should imply --disable-icf". Halp!

Comment 4

[Mike Hommey [:glandium]]

Move --enable-debug and --disable-optimize above MOZ_COMPILER_OPTS and set MOZ_DISABLE_ICF=1

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: both --disable-optimize and --enable-debug should imply --disable-icf

Thanks! This seems to be working here: libxul.so is built with --icf=safe by default in debug builds, and no longer with this patch.

However, I am very scared by this patch moving sizable chunks of code by thousands of lines up in configure.in. I have no confidence that this won't break things, e.g. if that code relies on state that changes in between these two distant places.

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 746066 [details] [diff] [review]
both --disable-optimize and --enable-debug should imply --disable-icf

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

::: configure.in
@@ +2622,5 @@
> +dnl =
> +dnl = Debugging Options
> +dnl =
> +dnl ========================================================
> +MOZ_ARG_HEADER(Debugging and Optimizations)

You can remove this, it doesn't do anything anyways, and is now inacurate.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/11a3ee38afff

Comment 8

[:ehsan akhgari]

Backed out because this apparently breaks everything!

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe3fc1746cae

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

This likely just needs a clobber.

Comment 10

[Phil Ringnalda (:philor)]

"Just needs a clobber" made sense to me, until https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=883e35db4839 came along, built Mac debug with a clobber, and still crashed both the build during leaktests and all the tests. Linux debug also seems to have crashed during startupcache precompilation (which is before packaging, so it didn't get to run tests that also crashed).

Comment 11

[Phil Ringnalda (:philor)]

And I retriggered a Mac build on your push after I set it to clobber, back when I thought it was a Mac-only problem, and that one was also exactly as busted as an unclobbered build+tests.

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

Ah... OK :-( Thanks for trying / for the information. Will look.

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Benoit you should reland this.

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

By all means, I wouldn't deprive you of that pleasure. Also, I'm off most tomorrow and on Tuesday.

Comment 15

[:ehsan akhgari]

Benoit, did you ever look into what caused the bustage?

Comment 16

[Benoit Jacob [:bjacob] (mostly away)]

No, not carefully.

Comment 17

[Gregory Szorc [:gps]]

glandium: could you please triage this bug?

Comment 18

[Mike Hommey [:glandium]]

This was essentially made moot by bug 904979.