Page MenuHomeFreeBSD

Bump LLVM_DEFAULT to 90
Needs ReviewPublic

Authored by jbeich on Tue, Aug 6, 8:35 PM.

Details

Reviewers
tobik
Test Plan

poudriere bulk -t is green on 11.2 amd64

Diff Detail

Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 25957
Build 24515: arc lint + arc unit

Event Timeline

jbeich created this revision.Tue, Aug 6, 8:35 PM
zeising added a subscriber: zeising.Tue, Aug 6, 8:47 PM
jbeich edited the test plan for this revision. (Show Details)EditedWed, Aug 7, 7:05 AM
jbeich added a reviewer: tobik.
jbeich added a subscriber: tobik.
jbeich updated this revision to Diff 60532.

@tobik, can you review how LLVM_DEFAULT is modified?
8/36 aka 22% of consumers aren't compatible:

tobik requested changes to this revision.Wed, Aug 7, 8:20 AM
tobik added inline comments.
security/afl++/Makefile
50–52

I think it is kind of ugly and confusing to set LLVM_DEFAULT like this. Ports in general should not set any *_DEFAULT.

Anyway it is not right with DEFAULT_VERSIONS+=llvm=-devel:

make: "/home/tobias/ports/llvm/security/afl++/Makefile" line 52: warning: String comparison operator should be either == or !=
make: "/home/tobias/ports/llvm/security/afl++/Makefile" line 52: Malformed conditional (${LLVM_DEFAULT} >= 90 || ${LLVM_DEFAULT} == "-devel")
make: Fatal errors encountered -- cannot continue
make: stopped in /home/tobias/ports/llvm/security/afl++

You could swap the comparisons, string first then integer, which solves the errors but will still show the warnings. Maybe do it like this instead

.if ${LLVM_DEFAULT:S,-devel,100,} >= 90
LLVM_DEFAULT=	80
.endif

As for afl++ and ccls could you share the failure logs so that I can send them upstream?

textproc/castxml/Makefile
32

LLVM_40 is stale. devel/llvm40 was removed a while ago.

This revision now requires changes to proceed.Wed, Aug 7, 8:20 AM
tobik added inline comments.Wed, Aug 7, 8:51 AM
devel/ccls/Makefile
41–46

Should no longer be needed after rP508301.

security/afl/Makefile
7

security/afl is not ready as even a basic make test fails. It would probably be best to lock it to llvm80 too.

jbeich marked 2 inline comments as done.EditedWed, Aug 7, 9:05 AM
jbeich updated this revision to Diff 60535.
  • Unbreak DEFAULT_VERSIONS+=llvm=-devel support
  • Rebase after rP508301 and rP508303
jbeich added inline comments.Wed, Aug 7, 9:06 AM
security/afl++/Makefile
50–52

Ports in general should not set any *_DEFAULT.

Short of introducing USES=llvm (a la USES=python) there's no other way to limit range.

.if ${LLVM_DEFAULT:S,-devel,100,} >= 90

-devel is supposed to be always greater, so I've used 990. Given current cadence of 2 major releases per year it won't need to be changed for ~45 years. ;)

As for afl++ and ccls could you share the failure logs so that I can send them upstream?

See P287 and P288 (previous version).

jbeich updated this revision to Diff 60536.Wed, Aug 7, 9:13 AM
tobik added inline comments.Wed, Aug 7, 9:30 AM
security/afl++/Makefile
50–52
jbeich updated this revision to Diff 60537.EditedWed, Aug 7, 9:42 AM
  • Per @tobik: limit security/afl to llvm < 90 due to make test failure
jbeich marked an inline comment as done.Wed, Aug 7, 9:43 AM
jbeich added a subscriber: jmd.EditedWed, Aug 7, 9:55 AM

@jmd or @zeising, can you update devel/libclc to a more recent LLVM snapshot? lang/clover (P294) compiles fine against LLVM 9 API while linking errors maybe unrelated.

@jmd or @zeising, can you update devel/libclc to a more recent LLVM snapshot? lang/clover (P294) compiles fine against LLVM 9 API while linking errors maybe unrelated.

https://reviews.freebsd.org/D21019

P294 updated: neither D21019 nor D19100 helps.

jbeich updated this revision to Diff 60540.Wed, Aug 7, 11:20 AM
tobik accepted this revision.Wed, Aug 7, 11:23 AM
This revision is now accepted and ready to land.Wed, Aug 7, 11:23 AM
tobik added a comment.Wed, Aug 7, 2:55 PM

@jbeich Any idea what is going on with P294 and the spam in the Phabricator feed? It happens every 5 minutes or so. Some automation gone awry maybe?

jbeich added a comment.Wed, Aug 7, 3:23 PM

@tobik, still no clue what you mean. I only get self-action spam despite turning it off in phabricator settings.

tobik added a comment.Wed, Aug 7, 3:29 PM

@tobik, still no clue what you mean. I only get self-action spam despite turning it off in phabricator settings.

See https://reviews.freebsd.org/feed/ and the bazillion "jbeich edited P294" messages. But I'm guessing this is something for the Phabricator admins to fix.

tobik added inline comments.Thu, Aug 8, 11:43 AM
security/afl++/Makefile
50–52

This can be removed again after rP508373.

jbeich updated this revision to Diff 60575.Thu, Aug 8, 12:45 PM
This revision now requires review to proceed.Thu, Aug 8, 12:45 PM
jbeich updated this revision to Diff 60576.Thu, Aug 8, 12:48 PM
jbeich marked 2 inline comments as done.Thu, Aug 8, 12:50 PM
jbeich updated this revision to Diff 60586.Thu, Aug 8, 4:16 PM
jbeich updated this revision to Diff 60592.Thu, Aug 8, 5:31 PM

Rather than overriding the global variable LLVM_DEFAULT I would prefer to see the following usage:

Index: devel/ispc/Makefile

  • devel/ispc/Makefile (revision 507746)

+++ devel/ispc/Makefile (working copy)
@@ -15,13 +15,14 @@
ONLY_FOR_ARCHS= amd64 i386
ONLY_FOR_ARCHS_REASON= only available for x86 architectures

-LIB_DEPENDS= libLLVM.so:devel/llvm${LLVM_DEFAULT}
+LLVM_VER= ${LLVM_DEFAULT}
+LIB_DEPENDS= libLLVM.so:devel/llvm${LLVM_VER}

USES= bison cmake python:build shebangfix
USE_GITHUB= yes
SHEBANG_FILES= *.py

-CONFIGURE_ENV= PATH=${LOCALBASE}/llvm${LLVM_DEFAULT}/bin:${PATH}
+CONFIGURE_ENV= PATH=${LOCALBASE}/llvm${LLVM_VER}/bin:${PATH}
CMAKE_OFF= ISPC_INCLUDE_EXAMPLES

BINARY_ALIAS= python=${PYTHON_CMD}
@@ -41,4 +42,10 @@

./${e}

.endfor

+.include <bsd.port.options.mk>
+
+.if ${LLVM_DEFAULT:S,-devel,990,} >= 90
+LLVM_VER= 80
+.endif
+
.include <bsd.port.mk>

If desired, I can apply this patch to the 5 ports involved and re-upload.

For related work, please see the wiki page https://wiki.freebsd.org/HardcodedLLVMVersions, in particular, case 1.

jbeich added a comment.Fri, Aug 9, 5:35 PM

If desired, I can apply this patch to the 5 ports involved and re-upload.

  • If an extra variable is necessary it should be in all consumers, not busted only
  • LLVM_DEFAULT compatibility is restricted here only temporary, until fixes land
  • During llvm100 switch (around 2020Q1) different consumers will likely break

Again, until USES=llvm or similar is introduced let's avoid churn. Some other *_DEFAULT variables also lack USE or USES e.g. JULIA_DEFAULT and RUST_DEFAULT.

jbeich updated this revision to Diff 60997.EditedMon, Aug 19, 3:40 PM