Since bug 1647780 landed, basic GDB support in the JS shell stopped working (even the most basic symbols are stripped away). In bug 1659452 we disabled stripping entirely as a workaround, but that leaves much more in the binary than is required in general (and leads to a huge size increase).

Instead we should probably support a stripping variant for the JS shell that behaves like it did before bug 1647780 landed. GDB support is otherwise not possible without the crashreporter-symbols-full.zip archive which even larger and not created for all builds.

Set release status flags based on info from the regressing bug 1647780

Christian -- I would appreciate a little more context here if the build team is meant to provide support.

Obviously bug 1659452 has been through a few iterations. This is the latest revision of that patch that was backed out, but the bug doesn't provide any insight or context about why the backout was necessary. What did this patch break? (I could do a try push myself and try to divine a failing job and piece it together myself, but that'll take longer.)

(In reply to Ricky Stewart from comment #2)

Christian -- I would appreciate a little more context here if the build team is meant to provide support.

Obviously bug 1659452 has been through a few iterations. This is the latest revision of that patch that was backed out, but the bug doesn't provide any insight or context about why the backout was necessary.

Bug 1659452 is actually fixed and is not the problem here. That bug disabled all stripping for the JS shell to work around a regression introduced by bug 1647780.

Bug 1647780 had the pretty bad side effect that it caused more stripping of the JS shell, up to the point where GDB cannot display any function names anymore. Leaving that information unstripped in the binary does not consume a lot of additional size (and we only need this for Nightly), but the build system and the script of the call patched in bug 1659452 do not allow more-fine grained stripping.

Does that clarify the problem?

Err -- no, it doesn't really clarify the problem. I'm already clear on the timeline. Repeating the question again:

Obviously bug 1659452 has been through a few iterations. This is the latest revision of that patch that was backed out, but the bug doesn't provide any insight or context about why the backout was necessary. What did this patch break? (I could do a try push myself and try to divine a failing job and piece it together myself, but that'll take longer.)

The reason I ask is that evidently you already tried to solve this problem (in bug 1659452), but the patch that I linked didn't work and was backed out. So I'm asking for additional clarity on why that failed. I don't want to just follow in your footsteps and land a patch that looks like yours and have it fail again for the same reason unbeknownst to me because the issues with the previous work were not documented.

(In reply to Ricky Stewart from comment #4)

The reason I ask is that evidently you already tried to solve this problem (in bug 1659452), but the patch that I linked didn't work and was backed out. So I'm asking for additional clarity on why that failed.

That approach failed because there is multiple configurations where we cannot use STRIP_FLAGS. One of them is when Valgrind is used (because that disables stripping already) and I tried to work around that with the various checks in the mozconfigs. But then we also figured out that coverage builds also won't work because they also disable some stripping and you get this:

[task 2020-08-18T21:55:48.082Z] 21:55:48    ERROR -  Traceback (most recent call last):
[task 2020-08-18T21:55:48.082Z] 21:55:48     INFO -    File "/builds/worker/checkouts/gecko/configure.py", line 183, in <module>
[task 2020-08-18T21:55:48.082Z] 21:55:48     INFO -      sys.exit(main(sys.argv))
[task 2020-08-18T21:55:48.082Z] 21:55:48     INFO -    File "/builds/worker/checkouts/gecko/configure.py", line 53, in main
[task 2020-08-18T21:55:48.082Z] 21:55:48     INFO -      sandbox.run(os.path.join(os.path.dirname(__file__), 'moz.configure'))
[task 2020-08-18T21:55:48.083Z] 21:55:48     INFO -    File "/builds/worker/checkouts/gecko/python/mozbuild/mozbuild/configure/__init__.py", line 457, in run
[task 2020-08-18T21:55:48.083Z] 21:55:48     INFO -      self._value_for(option)
[task 2020-08-18T21:55:48.083Z] 21:55:48     INFO -    File "/builds/worker/checkouts/gecko/python/mozbuild/mozbuild/configure/__init__.py", line 547, in _value_for
[task 2020-08-18T21:55:48.083Z] 21:55:48     INFO -      return self._value_for_option(obj)
[task 2020-08-18T21:55:48.083Z] 21:55:48     INFO -    File "/builds/worker/checkouts/gecko/python/mozbuild/mozbuild/util.py", line 1018, in method_call
[task 2020-08-18T21:55:48.083Z] 21:55:48     INFO -      cache[args] = self.func(instance, *args)
[task 2020-08-18T21:55:48.083Z] 21:55:48     INFO -    File "/builds/worker/checkouts/gecko/python/mozbuild/mozbuild/configure/__init__.py", line 615, in _value_for_option
[task 2020-08-18T21:55:48.083Z] 21:55:48     INFO -      % option_string.split('=', 1)[0])
[task 2020-08-18T21:55:48.084Z] 21:55:48     INFO -  mozbuild.configure.options.InvalidOptionError: STRIP_FLAGS is not available in this configuration
[task 2020-08-18T21:55:48.138Z] 21:55:48     INFO -  *** Fix above errors and then restart with\
[task 2020-08-18T21:55:48.139Z] 21:55:48     INFO -                 "./mach build"
[task 2020-08-18T21:55:48.139Z] 21:55:48     INFO -  client.mk:111: recipe for target 'configure' failed
[task 2020-08-18T21:55:48.139Z] 21:55:48     INFO -  make: *** [configure] Error 1

The problem with that approach is the way the various mozconfigs depend on each other, some with stripping already disabled, others not. One solution would be to just accept STRIP_FLAGS anyway when stripping was already disabled and make it a no-op. That would greatly simplify the configuration.

I don't know that the proposed fix is something we can realistically do. The "flag is not available in this configuration" checks are meaningful, and moving to just silently failing to honor user configs is a bad user experience. So the reason that I bring it up is that any patch for this issue is probably going to have to resemble the backed-out patch in question, so we might as well dig it up, find the problem, and fix it.

I'm doing a little investigation on that myself.

I did a try push of that patch, including some coverage builds, and the results look good -- namely, we're passing --strip-debug for all builds here, but not for coverage or fuzzing builds.

And furthermore I found the autoland push from when this patch was landed, and there are no failures, everything looks fine. I backfilled some ccov builds based on that revision and we'll see if it breaks.

Yeah, coverage builds on that autoland push are working fine. 🤷 Christian, is there some other issue with the patch that I'm not aware of? Otherwise do you want to put that patch back out for review and then re-land it?

(In reply to Ricky Stewart from comment #8)

Yeah, coverage builds on that autoland push are working fine. 🤷 Christian, is there some other issue with the patch that I'm not aware of? Otherwise do you want to put that patch back out for review and then re-land it?

So I time-traveled back in the logs to figure out why I requested backout of this:

<glandium> decoder: I bet https://hg.mozilla.org/integration/autoland/rev/14c0513e6bddcc99e870a83c7373cde280ce09b0 breaks artifact builds on automation
<decoder> glandium: I dont know anything about artifact builds. what would be your suggestion?
<glandium> decoder: remove the --strip from the js packaging line in upload_files.mk for now

Do you happen to know what will break here with artifact builds? Otherwise, we should ask glandium :)

This is very similar to this Phabricator revision, which was backed out. It works fine except it was breaking artifact builds, so we just fix that.

See patch.

https://hg.mozilla.org/mozilla-central/rev/2f2a6df81762