llvm@11: upstream Linux fixes #79466

danielnachun · 2021-06-17T05:05:53Z

Have you followed the guidelines for contributing?
Have you ensured that your commits follow the commit style guide?
Have you checked that there aren't other open pull requests for the same formula update/change?
Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

carlocab · 2021-06-17T19:28:17Z

Formula/llvm@11.rb

+    on_linux do
+      # Strip executables/libraries/object files to reduce their size
+      system("strip", "--strip-unneeded", "--preserve-dates", *(Dir[bin/"**/*", lib/"**/*"]).select do |f|
+        f = Pathname.new(f)
+        f.file? && (f.elf? || f.extname == ".a")
+      end)
+    end


Their build scripts generate an install/strip target. Wouldn't it be better to call that target instead on Linux? Or maybe calling it on both, though I'd want to know what this changes on macOS.

I think this might be carried over from the older versions where we weren't using the monorepo to build. I'll try to test this locally and see if it attains the same result on Linux. On macOS I can call it as well, and if it passes all the tests fine then maybe it's okay to use on both?

Sounds good. We can also compare the existing bottle to the newly built one to see if something looks off.

I built LLVM 12 locally with install/strip as the target and the bottle size only differs by 7 MB between the version built with install/strip and the original one we manually stripped. I also check some of the binaries produced by install/strip and file reports them all to be stripped.

Is there anything else we should be checking for comparison? I'm happy to build LLVM 11 and 12 bottles in the linuxbrew/core CI so that the artifacts can be downloaded to be compared if that would be useful.

carlocab · 2021-06-21T01:12:53Z

Formula/llvm@11.rb

@@ -132,6 +133,9 @@ def install
      -DLLDB_PYTHON_RELATIVE_PATH=libexec/#{site_packages}
      -DLIBOMP_INSTALL_ALIASES=OFF
      -DCLANG_PYTHON_BINDINGS_VERSIONS=#{py_ver}
+      -DPACKAGE_VENDOR=#{tap.user}
+      -DPACKAGE_BUGREPORT=#{tap.issues_url}
+      -DCLANG_VENDOR_UTI=org.#


Suggested change

-DCLANG_VENDOR_UTI=org.#

-DCLANG_VENDOR_UTI=org.#{tap.user.downcase}.clang

Whoops, had a copy/paste issue with my editor for that one.

carlocab · 2021-06-21T01:13:45Z

Formula/llvm@11.rb

@@ -146,6 +150,7 @@ def install
      args << "-DLLVM_BUILD_LLVM_C_DYLIB=ON"
      args << "-DLLVM_ENABLE_LIBCXX=ON"
      args << "-DLLVM_CREATE_XCODE_TOOLCHAIN=#{MacOS::Xcode.installed? ? "ON" : "OFF"}"
+      args << "-DRUNTIMES_CMAKE_ARGS=-DCMAKE_INSTALL_RPATH=#{rpath}"


Suggested change

args << "-DRUNTIMES_CMAKE_ARGS=-DCMAKE_INSTALL_RPATH=#{rpath}"

This isn't needed. The change in LC_RPATH entries for runtimes changed (i.e. regressed) in LLVM 12.

danielnachun · 2021-06-21T19:32:59Z

I don't understand this test failure. The most recent changes I pushed should not have affected it at all. Is this just another "temperamental" build issue?

carlocab · 2021-06-21T19:36:56Z

No, this is a test failure. LLVM 11 builds pretty reliably in my experience. Might be due to the newer SDK.

carlocab · 2021-06-21T19:40:58Z

The test appears to be failing here:

homebrew-core/Formula/llvm@11.rb

Line 289 in 58d4fbf

assert_equal "Hello World!", shell_output("./testlibc++").chomp

which is weird, because the SDK shouldn't have any effect on this.

carlocab · 2021-06-21T19:42:19Z

Formula/llvm@11.rb

+      refute_match(/libstdc\+\+/, lib)
+      refute_match(/libgcc/, lib)
+      refute_match(/libatomic/, lib)
+    end
    assert_equal "Hello World!", shell_output("./testlibc++").chomp


This is where the test is failing. I think the new flags you added to the compilation of testlibc++ aren't as innocuous as we thought.

I have had trouble with Compiler-RT on ARM in a separate PR, so it's possible it just doesn't work properly, and adding -rtlib=compiler-rt has exposed that problem.

I'm a little confused though, because I had a previous that was successful with that flag: https://github.com/Homebrew/homebrew-core/actions/runs/948629092. The one thing I did change was that I used the install/strip target. If that ends up creating a problem on ARM for compiler-rt, then that could indicate that install/strip breaks compiler-rt on ARM.

Indeed, going back to install from install/strip has fixed the test on ARM. I am thinking we should not use install/strip on ARM for LLVM 11, as compiler-rt may not be the only thing it breaks. I think LLVM 11 was the first version to support Apple Silicon, so perhaps the symbol stripping for that architecture had some problems which were resolved in LLVM 12?

Hmmm. Very weird. I suspect we'll want to avoid install/strip on LLVM 12 too. We can just go back to the on_linux block you wanted to do originally.

Probs not worth it, given that we don't have a clear idea of what it does and doesn't break. Especially given how we use LLVM_ENABLE_RUNTIMES, which means we have a bunch of things that aren't built with our shims and hence may contain things like debug symbols, etc.

I think that back then the difference between stripped and unstripped was bigger. And that I was manually building llvm and manually uploading it with a slow connection, like 10 to 20 times a year. One upload took between 1 and 2 hours, so every megabyte I could squeeze out of that build was worth it.

I am fine to remove the stripping.

It might be that those were Debug builds (cf. Homebrew/brew#11455), which is why stripping made a big difference.

Yeah if were unintentionally making them debug builds, removing the debugging symbols would make a much bigger difference than removing unused symbols. If we're not doing that anymore, that could probably explain why the effect is so small.

@carlocab with this resolved, I think we are ready to merge and nothing should need to be rebottled on macOS. ~~I'll have to open a new PR to disable stripping in LLVM 12, and~~ I won't add it for the older LLVM versions either.

@iMichka I think LLVM 11 and 12 should be revision bumped to disable stripping. Can that be handled in the merge? Or do we need to open new PRs for that? I'll change the PRs I opened for LLVM 7, 8, and 9 to disable stripping on those versions as well.

I see @carlocab already took of LLVM 12, so that I've crossed that out.

Using the `install/strip` target seems to have broken LLVM 11 on Apple Silicon (cf. Homebrew#79466). Stripping results in just a modest decrease in bottle size, so it seems better to play it safe here and stick with the `install` target.

Using the `install/strip` target seems to have broken LLVM 11 on Apple Silicon (cf. #79466). Stripping results in just a modest decrease in bottle size, so it seems better to play it safe here and stick with the `install` target.

danielnachun added the linux to homebrew-core Migration of linuxbrew-core to homebrew-core label Jun 17, 2021

BrewTestBot added legacy Relates to a versioned @ formula python Python use is a significant feature of the PR or issue labels Jun 17, 2021

carlocab reviewed Jun 17, 2021

View reviewed changes

danielnachun force-pushed the llvm@11 branch 2 times, most recently from 1d44fad to 60e8bc4 Compare June 21, 2021 01:10

carlocab reviewed Jun 21, 2021

View reviewed changes

danielnachun force-pushed the llvm@11 branch from 60e8bc4 to cc5f56a Compare June 21, 2021 03:33

carlocab reviewed Jun 21, 2021

View reviewed changes

danielnachun force-pushed the llvm@11 branch from cc5f56a to d242983 Compare June 22, 2021 00:41

llvm@11: upstream Linux fixes

38b6279

danielnachun force-pushed the llvm@11 branch from d242983 to 38b6279 Compare June 22, 2021 00:41

carlocab mentioned this pull request Jun 22, 2021

llvm: use install instead of install/strip target #79793

Merged

6 tasks

carlocab approved these changes Jun 23, 2021

View reviewed changes

danielnachun merged commit 72aac74 into Homebrew:master Jun 23, 2021

github-actions bot added the outdated PR was locked due to age label Jul 24, 2021

github-actions bot locked as resolved and limited conversation to collaborators Jul 24, 2021

danielnachun deleted the llvm@11 branch August 6, 2021 02:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

llvm@11: upstream Linux fixes #79466

llvm@11: upstream Linux fixes #79466

danielnachun commented Jun 17, 2021

carlocab Jun 17, 2021

danielnachun Jun 17, 2021

carlocab Jun 17, 2021

danielnachun Jun 18, 2021

carlocab Jun 21, 2021

danielnachun Jun 21, 2021

carlocab Jun 21, 2021

danielnachun commented Jun 21, 2021

carlocab commented Jun 21, 2021 •

edited

carlocab commented Jun 21, 2021

carlocab Jun 21, 2021

carlocab Jun 21, 2021

danielnachun Jun 22, 2021

danielnachun Jun 22, 2021 •

edited

carlocab Jun 22, 2021

carlocab Jun 22, 2021

iMichka Jun 22, 2021

carlocab Jun 22, 2021

danielnachun Jun 23, 2021 •

edited

danielnachun Jun 23, 2021

	-DCLANG_VENDOR_UTI=org.#
	-DCLANG_VENDOR_UTI=org.#{tap.user.downcase}.clang

llvm@11: upstream Linux fixes #79466

llvm@11: upstream Linux fixes #79466

Conversation

danielnachun commented Jun 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielnachun commented Jun 21, 2021

carlocab commented Jun 21, 2021 • edited

carlocab commented Jun 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielnachun Jun 22, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielnachun Jun 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlocab commented Jun 21, 2021 •

edited

danielnachun Jun 22, 2021 •

edited

danielnachun Jun 23, 2021 •

edited