Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CHERI LLVM updates #179

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

CHERI LLVM updates #179

wants to merge 17 commits into from

Conversation

brooksdavis
Copy link
Member

Pull in upstream patches to the CHERI LLVM ports.

The cherry-pick of ffb78cd is probably the biggest concern as it adds back std*.h headers. In the testing I was able to do on ordinary FreeBSD I didn't encounter issues and I've received no reports, but it may well be that we'll hit issues in CheriBSD ports builds.

Update to the latest snapshots.

We've done OK so far with installing standard headers as part of
devel/llvm## ports so extend that to these ports.  This is certainly
correct for llvm-cheriot.  Practical testing is likely required for the
others.

(cherry picked from commit ffb78cd)
Arm has merged LLVM 15 changes into Morello LLVM.

Sponsored by:	DARPA, AFRL

(cherry picked from commit 9584352)
Sponsored by:	DARPA, AFRL

(cherry picked from commit c85e97b)
Align license section with other devel/llvm* ports.

Only add per-LLVM release patches if any exist.

Sponsored by:	DARPA, AFRL

(cherry picked from commit f661b45)
The CHERIoT LLVM baseline has been updated to LLVM 17.

Sponsored by:	DARPA, AFRL

(cherry picked from commit 56f3450)
@kwitaszczyk
Copy link
Member

kwitaszczyk commented Jan 10, 2025

multimedia/libv4l was indeed crashing due to stddef.h being included from llvm-morello instead of the base system. When trying to build libv4l for hybrid using this toolchain version, I still get:

In file included from dvb-dev-remote.c:27:
/usr/include/execinfo.h:37:1: error: unknown type name '__BEGIN_DECLS'
__BEGIN_DECLS
^
/usr/include/execinfo.h:38:7: error: expected ';' after top level declarator
size_t backtrace(void **, size_t);
      ^
/usr/include/execinfo.h:43:1: error: unknown type name '__END_DECLS'
__END_DECLS
^

which is the same symptom as before.

@kwitaszczyk
Copy link
Member

I can see in upstream that devel/llvm17 installs standard headers but devel/llvm15 still does not. It might be worth checking if devel/llvm17 can build multimedia/libv4l on FreeBSD to know if this problem will go away once llvm-morello reaches 17.

@jrtc27
Copy link
Member

jrtc27 commented Jan 10, 2025

Including this header standalone without errors is exactly one of the reasons why stddef.h was previously removed from the package. Since most ports in FreeBSD will use the system compiler, not a devel/llvmN port, you're not going to notice most of the issues for FreeBSD. The headers should not be introduced upstream in any LLVM port, CHERI or otherwise, until a full exp run has been done using LLVM from ports as the system compiler on FreeBSD.

brooksdavis and others added 10 commits January 10, 2025 16:53
There are still some rough edges when used as a ports compiler.
These patches apply to LLVM 13 and should have been removed when
llvm-cheriot was updated.

Fixes:		56f3450 devel/llvm-cheriot: new LLVM 17 based snapshot
Sponsored by:	DARPA, AFRL

(cherry picked from commit 789df5a)
All LLVM ports in the tree use the Apache License 2.0 with LLVM
Exceptions license so cerntralize the definition.

Sponsored by:	DARPA, AFRL

(cherry picked from commit 18840af)
While most ports that need to override it can do so after including
Makefile.LICENSE, CHERI-LLVM derivatives benefit from being able to
pre-define it.

Sponsored by:	DARPA, AFRL

(cherry picked from commit fc0da5a)
All LLVM ports excluded it from the list as it was removed many years
ago.

Sponsored by:	DARPA, AFRL

(cherry picked from commit d17143d)
WRKSRC moved to the top of the source tree with llvm12.  As a result,
LICENSE_DIR became ${WRKSRC}/llvm.  Given that llvm11 the only port that
wasn't setting LICENSE_DIR, flip the script and default to
${WRKSRC}/llvm with llvm11 overriding.

Sponsored by:	DARPA, AFRL

(cherry picked from commit 4f177ec)
The location of this license file moved in LLVM 16.  Since I'm doing
cleanup in this area and the split of ports is about 50/50, switch to
setting the old location in the older ports so the newer ports set fewer
variables.

Sponsored by:	DARPA, AFRL

(cherry picked from commit 57f97b3)
Reported by:	dvl
Fixes:		57f97b3 devel/llvm*: flip LICENSE_FILE_BSD3CLAUSE setting [NFC]

(cherry picked from commit 1afdc80)
Move condiationl EXTRA_PATCHES additions after bsd.port.pre.mk so
PATCHDIR is defined.

Sponsored by:	DARPA, AFRL

(cherry picked from commit dab073e)
This adds clangd among other things.

(Due to the port structure, this includes a graturious PORTREVISION bump
in llvm-cheriot.  It doesn't seem worth the effort to avoid at this
point.)

(cherry picked from commit f7f5f91)
@brooksdavis
Copy link
Member Author

Including this header standalone without errors is exactly one of the reasons why stddef.h was previously removed from the package. Since most ports in FreeBSD will use the system compiler, not a devel/llvmN port, you're not going to notice most of the issues for FreeBSD. The headers should not be introduced upstream in any LLVM port, CHERI or otherwise, until a full exp run has been done using LLVM from ports as the system compiler on FreeBSD.

I've pushed a revert to the branch and more recently pushed an update to add clang-tools-extras.

Upstream I think the current state of affairs is a better balance given there's no supported, practical way to replace the base compiler with one of the llvm ports.

@brooksdavis
Copy link
Member Author

multimedia/libv4l was indeed crashing due to stddef.h being included from llvm-morello instead of the base system. When trying to build libv4l for hybrid using this toolchain version, I still get:

In file included from dvb-dev-remote.c:27:
/usr/include/execinfo.h:37:1: error: unknown type name '__BEGIN_DECLS'
__BEGIN_DECLS
^
/usr/include/execinfo.h:38:7: error: expected ';' after top level declarator
size_t backtrace(void **, size_t);
      ^
/usr/include/execinfo.h:43:1: error: unknown type name '__END_DECLS'
__END_DECLS
^

which is the same symptom as before.

In this specific case I think I've convinced myself that it's a bug that execinfo.h relies on header pollution to include sys/cdefs.h for __BEGIN_DECLS. It's an interesting question how many more things do this.

@jrtc27
Copy link
Member

jrtc27 commented Jan 10, 2025

multimedia/libv4l was indeed crashing due to stddef.h being included from llvm-morello instead of the base system. When trying to build libv4l for hybrid using this toolchain version, I still get:

In file included from dvb-dev-remote.c:27:
/usr/include/execinfo.h:37:1: error: unknown type name '__BEGIN_DECLS'
__BEGIN_DECLS
^
/usr/include/execinfo.h:38:7: error: expected ';' after top level declarator
size_t backtrace(void **, size_t);
      ^
/usr/include/execinfo.h:43:1: error: unknown type name '__END_DECLS'
__END_DECLS
^

which is the same symptom as before.

In this specific case I think I've convinced myself that it's a bug that execinfo.h relies on header pollution to include sys/cdefs.h for __BEGIN_DECLS. It's an interesting question how many more things do this.

It's not clear to me. I think it would be justified for our stddef.h to stop including sys/cdefs.h and break such cases. But at the same time whether including stddef.h includes sys/cdefs.h or not shouldn't really vary between compilers used.

@brooksdavis
Copy link
Member Author

In this specific case I think I've convinced myself that it's a bug that execinfo.h relies on header pollution to include sys/cdefs.h for __BEGIN_DECLS. It's an interesting question how many more things do this.

It's not clear to me. I think it would be justified for our stddef.h to stop including sys/cdefs.h and break such cases. But at the same time whether including stddef.h includes sys/cdefs.h or not shouldn't really vary between compilers used.

Hmm, it looks like gcc's stddef.h includes sys/_types.h for FreeBSD and only FreeBSD which includes sys/cdefs.h (for no reason that I can see). FreeBSD's stddef.h includes sys/_types.h and directly includes sys/cdefs.h for __ISO_C_VISIBLE (__STDC_VERSION__ could be used instead) and __offsetof (could just use the builtin since that's what we do unconditionally in sys/cdefs.h). I see two options: we could try to upstream the sys/_types.h include to LLVM which would bring parity (I'm not sure if I would accept that if I were them as the file is currently quite clean unlike the gcc one) or we could remove sys/cdefs.h use in FreeBSD's stddef.h and fix the fallout. I think I'll take a stab at the latter and see what happens.

This should have been updated as part of the commit below and the
PORTREVISION bump did apply to all. I was working on two things related
to these ports (one of which didn't touch llvm-cheriot) and got mixed
up.

Reported by:	jrtc27
Fixes:		f7f5f91 devel/llvm-{cheri,morello}: add clang-tools-extras

(cherry picked from commit fce28cc)
llvm-debuginfo-analyzer isn't part of Clang, but was matched by the
overly broad pattern `-analyzer`.

Reported by:	jrtc27
Sponsored by:	DARPA, AFRL

(cherry picked from commit f3f26e7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants