brew icon indicating copy to clipboard operation
brew copied to clipboard

Support for `--debug-symbols` for macos

Open lukaso opened this issue 1 year ago • 18 comments

  • [x] Have you followed the guidelines in our Contributing document?
  • [x] Have you checked to ensure there aren't other open Pull Requests for the same change?
  • [x] Have you added an explanation of what your changes do and why you'd like us to include them?
  • [x] Have you written new tests for your changes? Here's an example.
  • [x] Have you successfully run brew style with your changes locally?
  • [x] Have you successfully run brew typecheck with your changes locally?
  • [x] Have you successfully run brew tests with your changes locally?

As discussed in #13604.

This is spike of a PR to add --debug-symbols support for MacOS.

Currently this works to produce symbols, however it has many limitations that would be worked on if this is in the right direction.

This has been tested on some formulas and works seamlessly (no changes in formulas needed).

Actions:

  • [x] Needs a new --debug-symbols flag added throughout
  • [x] Flag to force -s and implicitly do --keep-tmp
  • [x] refurbish_args to add -g on triggering a config option from the flag (currently hard coded)
  • [x] Checklist above needs to be gone through
  • [x] Tests need to be written for the appropriate functions
  • [x] Also sets -g on linux

The main issue (assuming above) from my perspective is having to find the right tmp directory which changes on each build. But even so, I suggest that is a nice to have and I would defer to a later iteration if it becomes important.

lukaso avatar Jul 25 '22 23:07 lukaso

I wonder if it might be nicer to build inside HOMEBREW_CACHE instead of HOMEBREW_TEMP so that it's easier to preserve the source directory for these builds.

That would definitely be more ideal. However, is that a major revamp of the build process (I think it's run inside a sandbox, which I'm not that familiar with how they work)?

lukaso avatar Jul 26 '22 11:07 lukaso

I'm not sure how to write tests for this kind of cross cutting feature. I looked for tests for --cc= and --keep-tmp (which were my models) and didn't see any. Open to suggestions.

Also, if you'd like me to rewrite the commit history or anything like that, let me know.

Hopefully this is all in the right direction! Having used it, it really is helpful.

lukaso avatar Jul 26 '22 18:07 lukaso

Great work so far!

Thanks!

I'm not sure how to write tests for this kind of cross cutting feature.

I'd suggest a single brew install --debug-symbols integration test.

It's not a great test, but done in 15f1ac8775f19908b3f484ccdb21a0dcfda0099f. It would be better if it checked that -g was indeed set and that prepare_debug_symbols was called. However, I don't think that's easily possible in an integration test.

lukaso avatar Jul 30 '22 10:07 lukaso

I wonder if it might be nicer to build inside HOMEBREW_CACHE instead of HOMEBREW_TEMP so that it's easier to preserve the source directory for these builds.

Done in 4b0d52ef62c9e964b2a570b9e9459f454f311255, 8b1eb32e99efa05d263c8508becc187b44c9d33b, 93132c6876a6484bae0436bdb3d93c6717bcf828 and 41a526546661f400011d0ea56f7daa60cec458af

lukaso avatar Aug 01 '22 22:08 lukaso

Looks good! I'd like to test this out first before approving, but I don't see any glaring issues at the moment.

Fantastic!

lukaso avatar Aug 02 '22 01:08 lukaso

Here's the error I'm seeing in the CI.

1st Try error in ./test/cmd/install_spec.rb:76:
expected block to output /\.debug/ to stdout, but output nothing
Diff:
@@ -1 +1 @@
-/\.debug/
+""


RSpec::Retry: 2nd try ./test/cmd/install_spec.rb:76
F
Error: brew install installs formulae with debug symbols

Failure/Error:
  expect { system_command("objdump", args: ["-h", "${HOMEBREW_CELLAR}/testball1/0.1/bin/test"]) }
    .to output(/\.debug/).to_stdout

  expected block to output /\.debug/ to stdout, but output nothing
  Diff:
  @@ -1 +1 @@
  -/\.debug/
  +""

lukaso avatar Aug 07 '22 14:08 lukaso

I've managed to get the linux version running. --debug-symbols doesn't allow any formulas to compile.

Separately, I noticed that the homebrew install recommends users to brew install gcc. I think this is bad advice since building formulas from source then doesn't work. I don't know if that's intended.

(I'm doing all this on an emulated x86_64 so everything goes at a snails pace).

From what I can tell, libtool shouldn't break because -g has been added, but it's not able to link. But I'll keep seeing if I can fix this.

lukaso avatar Aug 10 '22 04:08 lukaso

OK, I've reinstalled homebrew. The build failures were all due to having run brew install gcc. Without that, formulas build correctly. I've also verified that debug symbols are correctly added when running with --debug-symbols. However, some tests are failing.

lukaso avatar Aug 10 '22 06:08 lukaso

Great work so far @lukaso, sorry this isn't easier!

OK, I've reinstalled homebrew. The build failures were all due to having run brew install gcc. Without that, formulas build correctly. I've also verified that debug symbols are correctly added when running with --debug-symbols.

@carlocab @fxcoudert @Homebrew/linux any ideas why brew install gcc may break this?

However, some tests are failing.

Happy to answer questions here but unfortunately will need these all passing before we can merge.

MikeMcQuaid avatar Aug 10 '22 13:08 MikeMcQuaid

On the overall PR: it seems to be a lot of work for a build-from-source-only option, which is a configuration which is not our primary focus anyway. It adds to the maintenance burden by complexifying the brew code. Also, it only builds debug symbols for C and C++ source, I think?

As to why it fails with GCC, I do not know. Is it on linux only or on macOS as well? GCC generates different type of debug info than LLVM on macOS, so it might be that. But it's hard to know without any actual error message/debug trace/etc.

fxcoudert avatar Aug 10 '22 14:08 fxcoudert

On the overall PR: it seems to be a lot of work for a build-from-source-only option, which is a configuration which is not our primary focus anyway. It adds to the maintenance burden by complexifying the brew code. Also, it only builds debug symbols for C and C++ source, I think?

@fxcoudert given how many folks have asked for this over the years and it being <200 lines added (including docs and test): I'm 👍🏻 on it.

MikeMcQuaid avatar Aug 10 '22 14:08 MikeMcQuaid

@carlocab @fxcoudert @Homebrew/linux any ideas why brew install gcc may break this?

However, some tests are failing.

Happy to answer questions here but unfortunately will need these all passing before we can merge.

All tests except the linux version of the integration test are working, unless I do brew install gcc which causes a lot of tests to fail, nothing to do with the addition of --debug-symbols. I think this is a separate issue.

In terms of tests, I'm working on the integration test for linux.

As to why it fails with GCC, I do not know. Is it on linux only or on macOS as well? GCC generates different type of debug info than LLVM on macOS, so it might be that. But it's hard to know without any actual error message/debug trace/etc.

The installer does not recommend running brew install gcc on MacOS. I haven't tested it there, so I don't know if it causes problems there. The problems appear related to relocatable code.

For now I'll stay focused on this PR and by then I might be near an intel mac so I can run the tests with a reasonable speed, and might be able to figure out the gcc problem.

lukaso avatar Aug 11 '22 01:08 lukaso

Hi, I've tried and tried to create a valid integration test. I am now compiling a very simple program. However, I don't know how to get the cc call to include the correct flags to the compile. ENV.cc only returns clang (on Mac) and manually adding -g doesn't work.

I'm certain the flags actually generate the correct debug symbols (I've tested on both Mac and Linux), but I simply can't figure out a way to test that this occurs in the integration test. This is due to either my lack of knowledge of the testing framework, or that there aren't tests that actually compile code with all flags.

I think we have two options:

  1. very simple integration test that just shows that nothing errors on Mac or Linux. It also can show that on Mac the dysmutil is indeed called.
  2. someone provides me the code to compile test.c with full compile flags in the test scenario in which case the linux test will work and I can see if I can improve the MacOS test to be confident there are actually debug symbols.

lukaso avatar Aug 11 '22 03:08 lukaso

  1. very simple integration test that just shows that nothing errors on Mac or Linux. It also can show that on Mac the dysmutil is indeed called.

This seems sufficient on macOS.

I wonder if on Linux we could do something like e.g. reading for a known string containing a debugging symbols, checking strip is a no-op or something else from https://stackoverflow.com/questions/3284112/how-to-check-if-program-was-compiled-with-debug-symbols?

MikeMcQuaid avatar Aug 11 '22 08:08 MikeMcQuaid

  1. very simple integration test that just shows that nothing errors on Mac or Linux. It also can show that on Mac the dysmutil is indeed called.

This seems sufficient on macOS.

I wonder if on Linux we could do something like e.g. reading for a known string containing a debugging symbols, checking strip is a no-op or something else from https://stackoverflow.com/questions/3284112/how-to-check-if-program-was-compiled-with-debug-symbols?

@MikeMcQuaid, the linux objdump test works as desired. I simply can't get the test program to compile with debug symbols in the integration test.

lukaso avatar Aug 11 '22 08:08 lukaso

@lukaso Ah, I see. In that case, unless someone on @Homebrew/linux like @danielnachun or @iMichka can help in the next ~24h: could consider just erroring out if the argument is passed on Linux and documenting in the command documentation and (which I'll handle) a post-merge help wanted issue that someone using Linux can pick this up if desired?

MikeMcQuaid avatar Aug 11 '22 08:08 MikeMcQuaid

@lukaso Ah, I see. In that case, unless someone on @Homebrew/linux like @danielnachun or @iMichka can help in the next ~24h: could consider just erroring out if the argument is passed on Linux and documenting in the command documentation and (which I'll handle) a post-merge help wanted issue that someone using Linux can pick this up if desired?

I think I'm still not quite explaining properly: The feature works perfectly on both Mac and Linux. There are no functionality issues.

It's the tests that are the problem. In fact, the test does not test anything on either platform (it doesn't test anything because the compiler is not being called with the right flags and I don't know how to make it do it).

My recommendation is to give up on a substantive test.

I hope that makes sense and sorry I've not explained well enough.

lukaso avatar Aug 11 '22 08:08 lukaso

My recommendation is to give up on a substantive test.

Gotcha, works for me. If you can walk through how I'd do a manual test to verify this locally (on macOS ARM), remove the test from this PR: I'm happy to ✅ and 🚢.

MikeMcQuaid avatar Aug 11 '22 10:08 MikeMcQuaid

My recommendation is to give up on a substantive test.

Gotcha, works for me. If you can walk through how I'd do a manual test to verify this locally (on macOS ARM), remove the test from this PR: I'm happy to ✅ and 🚢.

Excellent. I'll leave in the tests what I think is useful, and delete the rest.

Testing the feature

To test on ARM MacOS (or any mac) do:

brew install --debug-symbols --build-from-source libcsv
dsymutil -s <Cellar location>/Cellar/libcsv/3.0.3/lib/libcsv.3.dylib | grep N_OSO

Output should not be empty. Mine looks like:

[     4] 000001ce 66 (N_OSO        ) 00     0001   0000000000000000 '/Users/user/Library/Caches/Homebrew/Sources/libcsv/libcsv-3.0.3/.libs/libcsv_la-libcsv.o'

On Linux:

brew install --debug-symbols --build-from-source libcsv
objdump -h <Cellar location>/Cellar/libcsv/3.0.3/lib/libcsv.3.dylib | grep \.debug

Output should not be empty.

In both cases

brew reinstall --build-from-source libcsv

And then running the tests should return empty.

lukaso avatar Aug 11 '22 20:08 lukaso

No idea why installing Homebrew GCC breaks this. I don't see what it changes, since formulae should still continue to be built with the host compiler (unless they have fails_with, but those wouldn't have worked at all without Homebrew GCC).

carlocab avatar Aug 11 '22 23:08 carlocab

Thanks so much for your first contribution and sorry this took so long! Without people like you submitting PRs we couldn't run this project. You rock, @lukaso!

MikeMcQuaid avatar Aug 12 '22 08:08 MikeMcQuaid

Thanks Mike, that means a lot to me. I’ve been a devoted user of homebrew for a long time so feels good to give back!

lukaso avatar Aug 12 '22 08:08 lukaso