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

No Lint Coverage

Unit

No Test Coverage

Build Status

Buildable 55663
Build 52552: arc lint + arc unit

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

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
2657	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

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
2657	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
2657	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
2657	Well, the create the file in before patch or something, write it in WRKDIR somewhere.

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`.

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

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

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

@brooks You might be interested in the LLDB change.

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.

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.

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

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.

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

Assuming it works lgtm

This revision is now accepted and ready to land.Feb 2 2024, 11:20 AM

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..

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

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 133458

Mk/Features/debuginfo.mk

Mk/Scripts/generate-symbols.sh

Mk/Scripts/qa.sh

Mk/Uses/cmake.mk

Mk/Uses/meson.mk

Mk/bsd.port.mk

devel/gdb/Makefile

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

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 133458

Mk/Features/debuginfo.mk

Mk/Scripts/generate-symbols.sh

Mk/Scripts/qa.sh

Mk/Uses/cmake.mk

Mk/Uses/meson.mk

Mk/bsd.port.mk

devel/gdb/Makefile

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

Revision Contents
Changeset List