Page MenuHomeFreeBSD
Differential D43515

Features/debuginfo.mk: Automatically generate a subpackage with debugging info.
ClosedPublic
Actions

Authored by arrowd on Jan 19 2024, 6:30 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Dec 2, 1:59 PM
Unknown Object (File)
Sun, Dec 1, 6:32 AM
Unknown Object (File)
Sat, Nov 30, 10:20 PM
Unknown Object (File)
Sat, Nov 30, 9:18 PM
Unknown Object (File)
Tue, Nov 19, 12:12 AM
Unknown Object (File)
Thu, Nov 14, 11:13 PM
Unknown Object (File)
Wed, Nov 13, 4:41 PM
Unknown Object (File)
Wed, Nov 13, 9:16 AM
View All 50 Files
Subscribers
bapt
brooks
emaste
flo
mat
tcberner

Details

Reviewers
flo
tcberner
Group Reviewers
portmgr
Commits
R11:2c24a73ff2ca: Features/debuginfo.mk: Automatically generate a subpackage with debugging info.
Summary

I'm completely open to the bikeshedding process. The two concerns I'm aware of are

  • How to name the subpackage? I'm proposing debuginfo to underline the fact that the package installs only a debugging info files, not a debug flavor of the software itself.

My shell skills are lacking, so there probably also a room for improvement.

Test Plan

Tested with a CMake port sysutils/plasma5-powerdevil and a
simple port sysutils/44bsd-more. Also tested WITH_DEBUGINFO+WITH_DEBUG
combination.

Diff Detail

Repository
R11 FreeBSD ports repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

arrowd requested review of this revision.Jan 19 2024, 6:30 PM
arrowd created this revision.
Harbormaster completed remote builds in B55501: Diff 133054.Jan 19 2024, 6:30 PM
bapt added a subscriber: emaste.Jan 19 2024, 9:56 PM
bapt added a comment.Jan 19 2024, 10:00 PM

I don t see anything about cflags in the review am I missing something?

flo requested changes to this revision.Jan 19 2024, 10:01 PM
flo added a subscriber: flo.
flo added inline comments.
Mk/Features/debuginfo.mk
8

Typo.
WITH_DEBUGthe

Mk/bsd.port.mk
2653

Something similar needs to be added to Scripts/create-manifest.sh line 89, or rather we need a global pkg-descr for the debuginfo subpackages I think.

Found this while testing with asterisk18

'===> Checking if asterisk18 is already installed
cp: /usr/home/flo/dev/checkouts/portscommit/net/asterisk18/pkg-descr.debuginfo: No such file or directory

This revision now requires changes to proceed.Jan 19 2024, 10:01 PM
arrowd added a comment.Jan 20 2024, 7:02 AM
In D43515#992207, @bapt wrote:

I don t see anything about cflags in the review am I missing something?

I hideosly included Features/debug.mk, see the comment.

mat added a subscriber: mat.Jan 20 2024, 5:48 PM
mat added inline comments.
Mk/bsd.port.mk
2653

I'd say DESCR.debuginfo should be defined in the feature, and auto-generated during the staging process, and then contain Debug informations for ${PKGNAME} or something like that.

arrowd added inline comments.Jan 20 2024, 5:55 PM
Mk/bsd.port.mk
2653

Well, this check happens long before stage stage. So the file would still be non-existant.

... but maybe we keel this special-case check but still define DESCR.debuginfo to an autogenerated file and then it should probably work by the package stage?

mat added inline comments.Jan 20 2024, 6:22 PM
Mk/bsd.port.mk
2653

Well, the create the file in before patch or something, write it in WRKDIR somewhere.

arrowd updated this revision to Diff 133231.Jan 23 2024, 8:41 PM
arrowd marked 3 inline comments as done.
  • Create DESCR.debuginfo on the fly.
  • Fix typo.
Harbormaster completed remote builds in B55578: Diff 133231.Jan 23 2024, 8:41 PM
mat added inline comments.Jan 23 2024, 9:27 PM
Mk/Features/debuginfo.mk
30

I thought you said stage was too late for creating that file.

arrowd marked an inline comment as done.Jan 24 2024, 6:11 AM
arrowd added inline comments.
Mk/Features/debuginfo.mk
30

Turns out it works! But we still need .if !exists(${DESCR.${sp}}) with additional && ${sp} != debuginfo check in bsd.port.mk, otherwise it gets overriden with just category/port/pkg-descr.

emaste added a comment.Jan 24 2024, 9:49 PM

The current approach creates multiple .debug directories, while @bapt suggested to use ${LOCALBASE}/lib/debug to mirror /usr/lib/debug.

Note that debuggers will not search in ${LOCALBASE}/lib/debug by default, so although I think that's the "nicer" option I'd say using .debug to start is better.

emaste added inline comments.Jan 24 2024, 9:51 PM
Mk/Features/debuginfo.mk
6
arrowd updated this revision to Diff 133458.Jan 28 2024, 11:13 AM
arrowd marked an inline comment as done.
  • Reuse existing script for generating external debugging symbols.
  • devel/gdb: Point to LOCALBASE/lib/debug dir for the debugging symbols.
Harbormaster completed remote builds in B55663: Diff 133458.Jan 28 2024, 11:13 AM
arrowd updated this revision to Diff 133462.Jan 28 2024, 2:55 PM
  • devel/llvm15: Patch lldb to search for debug symbols in LOCALBASE/lib/debug.
Harbormaster completed remote builds in B55666: Diff 133462.Jan 28 2024, 2:55 PM
arrowd added a subscriber: brooks.Jan 28 2024, 2:57 PM

@brooks You might be interested in the LLDB change.

mat added a comment.Jan 28 2024, 3:08 PM
In D43515#994013, @emaste wrote:

The current approach creates multiple .debug directories, while @bapt suggested to use ${LOCALBASE}/lib/debug to mirror /usr/lib/debug.

Note that debuggers will not search in ${LOCALBASE}/lib/debug/ by default, so although I think that's the "nicer" option I'd say using .debug to start is better.

Well, debuggers should be taught to go look there. I'm not sure why we can't put files there because nobody looks there, of course nobody looks there, it does not exist yet ;-)
I have no idea how debuggers work, but they probably have some kind of PATH equivalent for looking for debug info, we just need to add /usr/local/lib/debug to it.

arrowd edited the summary of this revision. (Show Details)Jan 28 2024, 3:25 PM

The debuginfo is installed into ${LOCALBASE}/lib/debug/ now and I taught both GDB and LLDB to look into there.

brooks added inline comments.Jan 29 2024, 6:01 PM
devel/llvm15/files/patch-lldb_source_Symbol_LocateSymbolFile.cpp
25–27 ↗(On Diff #133462)

This is meaningful different from what is done with gdb. In the gdb case you hardcode the pkg set's LOCALBASE. Here you look up the system wide default. They aren't necessarily the same thing. (For example on CheriBSD we build pure-capability (memory safe) packages with LOCALBASE=/usr/local and a separate package set for ordinary 64-bit binaries with LOCALBASE=/usr/local64 and don't have a per-ABI user.localbase).

Given the lldb is a cross debugger I'm not sure this difference matters much so maybe we would just upstream this patch and not worry about it further.

arrowd added inline comments.Jan 29 2024, 6:03 PM
devel/llvm15/files/patch-lldb_source_Symbol_LocateSymbolFile.cpp
25–27 ↗(On Diff #133462)

This is because I want to put the same patch into our base LLDB.

Given the lldb is a cross debugger I'm not sure this difference matters much so maybe we would just upstream this patch and not worry about it further.

Yes, I also wanted to upstream this, if you think it is OK.

flo accepted this revision.Jan 29 2024, 7:01 PM

My issues have been resolved. LGTM. I was able to successfully test this.

This revision is now accepted and ready to land.Jan 29 2024, 7:01 PM
emaste added a comment.Jan 29 2024, 7:13 PM

Well, debuggers should be taught to go look there.

Indeed, we should probably do that for lldb by implementing the equivalent of GDB's configure-time --with-additional-debug-dirs (that I just learned about from this review).

devel/llvm15/files/patch-lldb_source_Symbol_LocateSymbolFile.cpp
25–27 ↗(On Diff #133462)

For this case we probably want to search in both paths?

emaste added inline comments.Jan 29 2024, 7:30 PM
Mk/Features/debuginfo.mk
7

I assume this is fixed in your local tree?

brooks added inline comments.Jan 29 2024, 7:30 PM
devel/llvm15/files/patch-lldb_source_Symbol_LocateSymbolFile.cpp
25–27 ↗(On Diff #133462)

For this case we probably want to search in both paths?

Yeah, I think so.

arrowd updated this revision to Diff 133528.Jan 29 2024, 7:39 PM
arrowd marked 2 inline comments as done.
  • Fix typo.
This revision now requires review to proceed.Jan 29 2024, 7:39 PM
Harbormaster completed remote builds in B55693: Diff 133528.Jan 29 2024, 7:39 PM
arrowd added inline comments.Jan 29 2024, 7:39 PM
devel/llvm15/files/patch-lldb_source_Symbol_LocateSymbolFile.cpp
25–27 ↗(On Diff #133462)

Sorry, I didn't get it, what "both paths"?

brooks added inline comments.Jan 29 2024, 7:43 PM
devel/llvm15/files/patch-lldb_source_Symbol_LocateSymbolFile.cpp
25–27 ↗(On Diff #133462)

user.localbase and the value of LOCALBASE for a given package set aren't necessarily the same thing. Ideally lldb should look in both if they differ.

arrowd added inline comments.Jan 29 2024, 8:00 PM
devel/llvm15/files/patch-lldb_source_Symbol_LocateSymbolFile.cpp
25–27 ↗(On Diff #133462)

This is that LOCALBASE exist only when building from Ports. When LLDB is also built from base, we don't have this variable at the compile time.

emaste added inline comments.Jan 29 2024, 8:41 PM
devel/llvm15/files/patch-lldb_source_Symbol_LocateSymbolFile.cpp
25–27 ↗(On Diff #133462)

That said this issue does not need to be resolved in advance of this change going in, we can iterate on lldb (and gdb) separately

tcberner accepted this revision.Feb 2 2024, 11:20 AM
tcberner added a subscriber: tcberner.

Assuming it works lgtm

This revision is now accepted and ready to land.Feb 2 2024, 11:20 AM
emaste added a comment.Feb 2 2024, 9:10 PM

looks good, I'm very happy to see this happening

still an extract -> extracted to update

the lldb change should be split out and handled separately, and we need to address this in several places (upstream lldb, in the base system, multiple ports)

Closed by commit R11:2c24a73ff2ca: Features/debuginfo.mk: Automatically generate a subpackage with debugging info. (authored by arrowd). · Explain WhyFeb 3 2024, 6:32 AM
This revision was automatically updated to reflect the committed changes.
arrowd added a commit: R11:2c24a73ff2ca: Features/debuginfo.mk: Automatically generate a subpackage with debugging info..

Revision Contents
Changeset List

PathSize
Mk/
Features/
34 lines
Scripts/
17 lines
2 lines
Uses/
4 lines
2 lines
6 lines

Diff 133784

View Options

Mk/Features/debuginfo.mk

Loading...
View Options

Mk/Scripts/generate-symbols.sh

Loading...
View Options

Mk/Scripts/qa.sh

Loading...
View Options

Mk/Uses/cmake.mk

Loading...
View Options

Mk/Uses/meson.mk

Loading...
View Options

Mk/bsd.port.mk

Loading...