Page MenuHomeFreeBSD

Bump LLVM_DEFAULT to 90
ClosedPublic

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

Details

Test Plan

poudriere bulk -t is green on 11.2 amd64

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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

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

tobik requested changes to this revision.Aug 7 2019, 8:20 AM
tobik added inline comments.
security/afl++/Makefile
51–53 ↗(On Diff #60532)

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 ↗(On Diff #60532)

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

This revision now requires changes to proceed.Aug 7 2019, 8:20 AM
tobik added inline comments.Aug 7 2019, 8:51 AM
devel/ccls/Makefile
42–47 ↗(On Diff #60532)

Should no longer be needed after rP508301.

security/afl/Makefile
7 ↗(On Diff #60532)

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

jbeich updated this revision to Diff 60535.EditedAug 7 2019, 9:05 AM
jbeich marked 2 inline comments as done.
  • Unbreak DEFAULT_VERSIONS+=llvm=-devel support
  • Rebase after rP508301 and rP508303
jbeich added inline comments.Aug 7 2019, 9:06 AM
security/afl++/Makefile
51–53 ↗(On Diff #60532)

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.Aug 7 2019, 9:13 AM
tobik added inline comments.Aug 7 2019, 9:30 AM
security/afl++/Makefile
51–53 ↗(On Diff #60532)
jbeich updated this revision to Diff 60537.EditedAug 7 2019, 9:42 AM
  • Per @tobik: limit security/afl to llvm < 90 due to make test failure
jbeich marked an inline comment as done.Aug 7 2019, 9:43 AM
jbeich added a subscriber: jmd.EditedAug 7 2019, 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.Aug 7 2019, 11:20 AM
tobik accepted this revision.Aug 7 2019, 11:23 AM
This revision is now accepted and ready to land.Aug 7 2019, 11:23 AM
tobik added a comment.Aug 7 2019, 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.Aug 7 2019, 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.Aug 7 2019, 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.Aug 8 2019, 11:43 AM
security/afl++/Makefile
51–53 ↗(On Diff #60540)

This can be removed again after rP508373.

jbeich updated this revision to Diff 60575.Aug 8 2019, 12:45 PM
This revision now requires review to proceed.Aug 8 2019, 12:45 PM
jbeich updated this revision to Diff 60576.Aug 8 2019, 12:48 PM
jbeich marked 2 inline comments as done.Aug 8 2019, 12:50 PM
jbeich updated this revision to Diff 60586.Aug 8 2019, 4:16 PM
jbeich updated this revision to Diff 60592.Aug 8 2019, 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.Aug 9 2019, 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.EditedAug 19 2019, 3:40 PM
  • If an extra variable is necessary it should be in all consumers, not busted only

I have local patches to do this. But IMHO fixing these cases should come first.

  • LLVM_DEFAULT compatibility is restricted here only temporary, until fixes land

I don't understand this statement.

  • During llvm100 switch (around 2020Q1) different consumers will likely break

I don't understand why this should mean this change not go in.

Again, until USES=llvm or similar is introduced let's avoid churn.

Adding the USES=llvm code is a worthy goal but I don't have the cycles to do it ATM. These fixes seem to me to be needed in the meantime.

Some other *_DEFAULT variables also lack USE or USES e.g. JULIA_DEFAULT and RUST_DEFAULT.

So? There's only so many things I can try to fix at one time.

arrowd added a subscriber: arrowd.Aug 28 2019, 12:00 PM
arrowd added inline comments.
security/klee/Makefile
53 ↗(On Diff #60997)

Why not hardcode 80 here, instead of using LLVM_DEFAULT?

jbeich marked an inline comment as done.Aug 28 2019, 3:09 PM
  • If an extra variable is necessary it should be in all consumers, not busted only

I have local patches to do this. But IMHO fixing these cases should come first.

Inconsistency complicates maintenance. If all LLVM_DEFAULT consumers look the same it's easy to refactor.

  • LLVM_DEFAULT compatibility is restricted here only temporary, until fixes land

I don't understand this statement.

A number of consumers have already been fixed before the patch here landed. Dropping hacks involves removing a few lines instead of renaming LLVM_VER back to LLVM_DEFAULT.

Anyway, if you consider direct usage of LLVM_DEFAULT as hardcoded then just do the cleanup (or open separate differential for review) without trying to hijack an unrelated change. Make it atomic to minimize rebase churn and facilitate post-commit review. I'm not particularly attached to the current style.

security/klee/Makefile
53 ↗(On Diff #60997)

To keep support for older devel/llvm* e.g., DEFAULT_VERSIONS+=llvm=60.

jbeich updated this revision to Diff 61799.Sep 8 2019, 12:49 AM
jbeich marked an inline comment as done.
  • Rebase
This revision was not accepted when it landed; it landed in state Needs Review.Fri, Sep 20, 7:58 PM
This revision was automatically updated to reflect the committed changes.