beanz (Chris Bieneman)
User

Projects

User does not belong to any projects.

User Details

User Since
Aug 20 2014, 4:35 PM (220 w, 6 d)

Recent Activity

Yesterday

beanz added inline comments to D54443: [CMake] Accept ENTITLEMENTS in add_llvm_executable and llvm_codesign.
Tue, Nov 13, 2:19 PM
beanz added a comment to D54461: [CMake] Support cross-compiling with multi-stage builds.

One slightly nitpicky comment.

Tue, Nov 13, 2:17 PM
beanz accepted D54371: [CMake] Passthrough CFLAGS when checking the compiler-rt path.

LGTM

Tue, Nov 13, 10:04 AM

Mon, Nov 12

beanz added inline comments to D54443: [CMake] Accept ENTITLEMENTS in add_llvm_executable and llvm_codesign.
Mon, Nov 12, 1:43 PM
beanz added a comment to D54352: [CMake] Explicit lldb_codesign function with application in debugserver and lldb-server.

I can certainly foresee other LLVM-based projects needing entitlements, so I am very much in favor of adding ENTITLEMENTS and FORCE options onto llvm_codesign.

Mon, Nov 12, 9:58 AM

Fri, Nov 9

beanz added a comment to D54352: [CMake] Explicit lldb_codesign function with application in debugserver and lldb-server.

Why not just add entitlement support to ‘llvm_codesign’? Or feed through extra arguments?

Fri, Nov 9, 3:18 PM

Thu, Nov 8

beanz added a comment to D54153: Fix compilation issue in VS2017 with Clang-tablegen and LLVM-tablegen.

I’m very confused how this could happen with Ninja. All the CMake invocations are done in targets with ‘USES_TERMINAL’ set on them which puts them in the ‘console’ pool which only allows one task at a time. So there shouldn’t be a way to have more than one Ninja invocation running at a time.

Thu, Nov 8, 10:14 AM

Wed, Nov 7

beanz accepted D54236: [cmake] Set CMP0075 to NEW.

My biggest argument against changing how clang does things is that we want to get the policy failures there too so that we know to fix them. We do support building clang out of tree, and hiding the warnings for in-tree builds will just make it harder to find and fix them.

Wed, Nov 7, 4:07 PM

Tue, Nov 6

beanz accepted D53959: Remove working directory for debugserver code signing target.

LGTM

Tue, Nov 6, 2:55 PM
beanz added a comment to D53959: Remove working directory for debugserver code signing target.

Honestly, I don't know that we need to specify a working directory on this command at all. Since we're specifying the target file using a generator expression everything else should work because we use explicit not relative paths.

Tue, Nov 6, 9:32 AM

Mon, Oct 29

beanz added a comment to D53778: [CMAKE] Specify all_load when exporting symbols from an executable (macOS).

Adding -bundle_loader in add_llvm_loadable_module doesn't work because LLVM modules can be loaded by lots of different tools, so they actually need to be fully self-contained. Obviously if a specific distribution is only supporting the module from one tool that distribution can do things differently, but as a general rule we can't make that assumption.

Mon, Oct 29, 10:22 AM

Fri, Oct 26

beanz added a comment to D53778: [CMAKE] Specify all_load when exporting symbols from an executable (macOS).

I'm not really sure this is the best solution. This will result in bloating the clang binary substantially. An alternative approach would be to use linker order precedence, and pass the LLVM executable first, then LLVM static archives later on the link line. That would allow minimal tool binaries, and link the LLVM bits into your plugin.

Fri, Oct 26, 3:38 PM
beanz added a comment to D49672: [CMake] Honor LLVM_EXTERNAL_<proj>_SOURCE_DIR.

Yes, this is the preferred way to do this. No, we don’t need a big comment about it. The solution I described is how you define command-line configurable options in CMake that are not Boolean (for Boolean options CMake has the option function which is really just a wrapper around this pattern).

Fri, Oct 26, 9:21 AM

Mon, Oct 22

beanz added a comment to D49672: [CMake] Honor LLVM_EXTERNAL_<proj>_SOURCE_DIR.

I don't disagree with the intent of the patch, but this patch is *way* more complicated than it needs to be to accomplish the goal. All you should need to do is add CACHE STRING "" to the set command in line 137. The set command's CACHE option doesn't override existing values, it only sets them if they were not previously set.

Mon, Oct 22, 4:54 PM

Mon, Oct 15

beanz updated subscribers of D28791: [compiler-rt][crt] Simple crtbegin and crtend implementation.
Mon, Oct 15, 2:58 PM

Oct 10 2018

beanz created D53110: [Coverage] Apply filtered paths to summary.
Oct 10 2018, 2:39 PM

Oct 4 2018

beanz accepted D52199: [profile] Install headers for custom runtime maintainers.

LGTM.

Oct 4 2018, 8:25 PM

Sep 19 2018

beanz added inline comments to D51748: cmake: Remove add_llvm_loadable_module().
Sep 19 2018, 11:30 AM

Sep 13 2018

beanz added a comment to D51984: Fixes for `LLVM_LINK_LLVM_DYLIB` && Polly..

@lebedev.ri, I told him he could commit the changes without review because I saw them in this patch, and in my mind, as an experienced developer in this area of LLVM, I saw those changes as obvious and good. That is not in violation of the developer policy, further that discussion is right here in this review, so if you have a problem with it you should point it at me, not @DiamondLovesYou.

Sep 13 2018, 1:51 PM
beanz added a comment to D51984: Fixes for `LLVM_LINK_LLVM_DYLIB` && Polly..

If you add this to your patch here, the Clang patch should work as I've suggested:

diff --git a/cmake/modules/LLVM-Config.cmake b/cmake/modules/LLVM-Config.cmake
index 8eabddc7377..a306e41f26d 100644
--- a/cmake/modules/LLVM-Config.cmake
+++ b/cmake/modules/LLVM-Config.cmake
@@ -244,6 +244,9 @@ function(llvm_map_components_to_libnames out_libs)
           list(APPEND expanded_components "LLVM${t}Info")
         endif()
       endforeach(t)
+    elseif( c STREQUAL "Polly")
+      # LLVMPolly is the Polly loadable module target, the static archive is just Polly
+      list(APPEND expanded_components "${c}")
     else( NOT idx LESS 0 )
       # Canonize the component name:
       string(TOUPPER "${c}" capitalized)

It is unfortunate that Polly doesn't match the naming conventions of other LLVM components, but we have a lot of special case handling for this kind of thing anyways.

Sep 13 2018, 10:56 AM

Sep 12 2018

beanz added a comment to D51984: Fixes for `LLVM_LINK_LLVM_DYLIB` && Polly..

I think this should be split into multiple patches. The places you changed add_library to add_llvm_library should be split out. Also you can break out the unittests/Passes/CmakeLists.txt into its own patch. All of those are good cleanup, and you can commit them without review.

Sep 12 2018, 5:19 PM
beanz added a comment to D51986: Fixes for `LLVM_LINK_LLVM_DYLIB` && Polly..

I agree that changing the naming scheme probably makes sense, but I also think that you can probably make a fairly well contained change to the LLVM CMake module that maps components to libnames to special case Polly and make this all work.

Sep 12 2018, 5:11 PM
beanz added a comment to D51986: Fixes for `LLVM_LINK_LLVM_DYLIB` && Polly..

After line 18 in this file you could do something like:

Sep 12 2018, 1:57 PM
beanz added a comment to D51986: Fixes for `LLVM_LINK_LLVM_DYLIB` && Polly..

I don’t think this is the right solution. The build system knows what components are in the dylib and should remove them from the list of libraries linked individually. You should be able to make Polly behave like an LLVM component, then tools don’t need to care if the dylib is used or not.

Sep 12 2018, 12:15 PM

Sep 6 2018

beanz added inline comments to D51748: cmake: Remove add_llvm_loadable_module().
Sep 6 2018, 2:51 PM

Sep 4 2018

beanz accepted D51645: [CMake] Don't use -rtlib=compiler-rt with -nodefaultlibs..

LGTM.

Sep 4 2018, 1:58 PM
beanz updated subscribers of D51440: [ToolChains] Link to compiler-rt with -L + -l when possible.

Unfortunately I can't make this kind of policy decision about whether or not this would be acceptable for Darwin. That would probably need to be @dexonsmith.

Sep 4 2018, 11:20 AM
beanz accepted D51603: [CMake] Provide a custom target to install LLVM libraries.

LGTM! Didn't look back through enough context.

Sep 4 2018, 11:13 AM
beanz added a comment to D51603: [CMake] Provide a custom target to install LLVM libraries.

One small comment below. Otherwise LGTM.

Sep 4 2018, 11:09 AM

Aug 31 2018

beanz created D51551: [LLJIT] Add API to expose linking layer from LLJIT classes.
Aug 31 2018, 11:23 AM

Aug 29 2018

beanz added a reviewer for D51440: [ToolChains] Link to compiler-rt with -L + -l when possible: phosek.

Looping in @phosek.

Aug 29 2018, 10:19 AM
beanz accepted D51439: [LLD] [CMake] Add an lld-test-depends target.

LGTM. This is consistent with how we setup these targets for other projects.

Aug 29 2018, 10:18 AM

Aug 28 2018

beanz added a comment to D49485: CMake: use new policy for CMP0051.

Heh... yea, didn't notice this review. I removed the policy setting (which will default to NEW). @chapuni fixed the underlying issue that required us to have this enabled *years* ago.

Aug 28 2018, 2:27 PM

Aug 22 2018

beanz accepted D50547: [Driver] Use normalized triples for multiarch runtime path.

LGTM as-is

Aug 22 2018, 1:45 PM

Aug 20 2018

beanz added a comment to D50547: [Driver] Use normalized triples for multiarch runtime path.

Just want to bring an IRC conversation that I had with @phosek into the proper code review.

Aug 20 2018, 1:55 PM

Aug 16 2018

beanz accepted D50547: [Driver] Use normalized triples for multiarch runtime path.

LGTM!

Aug 16 2018, 10:58 AM

Aug 15 2018

beanz accepted D50755: [Driver] -print-target-triple and -print-effective-triple options.

lgtm

Aug 15 2018, 1:06 PM

Aug 12 2018

beanz created D50618: Refactor Darwin driver to refer to runtimes by component.
Aug 12 2018, 5:17 PM

Aug 9 2018

beanz added a comment to D49486: [cfe][CMake] Export the clang resource directory.

Please check and see if that works for you. In general I prefer to avoid adding new features that aren't used in-tree unless it is unavoidable or would be difficult to work around out-of-tree. This one seems pretty straightforward to me.

Aug 9 2018, 10:39 AM
beanz added a comment to D49486: [cfe][CMake] Export the clang resource directory.

I’m not opposed to this patch, but is there a real need for this? Have you considered just running clang with the -print-resource-dir flag?

Aug 9 2018, 9:16 AM

Aug 8 2018

beanz updated subscribers of D45639: [Driver] Support default libc++ library location on Darwin.

Adding @jfb since this is his domain now too.

Aug 8 2018, 1:19 PM
beanz accepted D39939: [cmake] Append LLVM_VERSION_SUFFIX to SOVERSION.

Looks good. Sorry for the extended delay in review.

Aug 8 2018, 1:15 PM
beanz accepted D43701: [cmake] Store LLVM_VERSION_SUFFIX in LLVMConfig.cmake.

Looks fine. Sorry for the extended delay in review.

Aug 8 2018, 1:14 PM

Aug 6 2018

beanz accepted D50034: Add a CommandGuide for llvm-objdump.

@mtrent and I talked offline, and he explained how the sphinx documentation covers the manpage and program directives, and I agree with his assertion that using manpage is correct.

Aug 6 2018, 3:40 PM

Aug 2 2018

beanz added a comment to D50034: Add a CommandGuide for llvm-objdump.

One last small fix

Aug 2 2018, 3:21 PM

Jul 31 2018

beanz added a comment to D50034: Add a CommandGuide for llvm-objdump.

Given how mach-o is radically different from ELF, it might make sense to sort or group ELF and mach-o specific options. Right now we only call out options that only work on Mach-o, but I assume some of the options only work on ELF and/or COFF. Might make sense to document that behavior correctly too.

Jul 31 2018, 10:59 AM

Jul 10 2018

beanz accepted D48797: [CMake] Teach the build system to codesign built products.

Looping in @davide and @JDevlieghere. LLDB has some hand-rolled goop in debug server's build that could potentially be replaced by this.

Jul 10 2018, 10:33 AM
beanz accepted D49121: [CMake] Set per-runtime library directory suffix in runtimes build.

LGTM

Jul 10 2018, 10:28 AM

Jun 26 2018

beanz accepted D48061: [CMake] Provide direct support for building sanitized runtimes.

LGTM!

Jun 26 2018, 12:09 PM

Jun 11 2018

beanz accepted D47355: [CMake] Allow specifying extra dependencies of bootstrap stage.

I prefer DEPS to COMPONENTS because I've tried to explicitly use the term components to mean targets that have paired build and install targets. Otherwise looks good.

Jun 11 2018, 2:01 PM

May 18 2018

beanz added a reviewer for D45604: Support for multiarch runtimes layout: bruno.

Looping in @bruno in case he has feedback.

May 18 2018, 9:57 AM

May 17 2018

beanz accepted D46857: [CMake] Detect the compiler runtime and standard library.

This all looks reasonable to me. I had a few small comments below. There are a few isolated bits of code cleanup that you included in this patch, and I'd prefer to see those land separately.

May 17 2018, 9:52 AM

May 10 2018

beanz created D46705: [CMake] Support runtimes in distributions.
May 10 2018, 10:18 AM

May 1 2018

beanz added a reviewer for D45639: [Driver] Support default libc++ library location on Darwin: bruno.

@dexonsmith is often super busy, so @bruno may be able to weigh in instead.

May 1 2018, 11:40 AM

Jan 31 2018

beanz accepted D42490: [cmake] Set cmake policy CMP0068 to suppress warnings on OSX.

Historically we've duplicated cmake_policy calls on a per-project basis which we needed to support standalone builds. That said, it would be nice if we had LLVM vend some CMake modules that encapsulated this stuff better. In general I think that cleanup would be ideal to do after we move LLVM to the GitHub mono-repo because then we can create a cmake/modules folder at the root of the mono-repo that has common modules useable by all sub-projects.

Jan 31 2018, 3:54 PM
beanz accepted D42238: [CMake] Remove -stdlib= which is unused when passing -nostdinc++.

LGTM

Jan 31 2018, 3:36 PM

Jan 9 2018

beanz accepted D41807: [cmake] Delete redundant install command for clang-refactor..

LGTM

Jan 9 2018, 3:19 PM

Jan 5 2018

beanz accepted D41272: Don't try to run MCJIT/OrcJIT EH tests when C++ library is statically linked.

This looks good to me. Please correct me if I'm wrong, but it also looks like you've written this patch so that it will work correctly even if it is landed before the llvm-readobj changes are landed. If that is the case go ahead and land this.

Jan 5 2018, 1:39 PM

Jan 3 2018

beanz added a reviewer for D41706: [Driver] Update default sanitizer blacklist location: kubamracek.

For reference this relates to D41673. Looking in @kubamracek.

Jan 3 2018, 10:46 AM
beanz accepted D41678: [CMake] Support for cross-compilation when build runtimes.

LGTM

Jan 3 2018, 10:32 AM
beanz added a comment to D41402: [cmake] Fix DESTDIR support in compiler-rt build.

I like @phosek's idea of moving the files into /share, but that needs some accompanying changes to clang to find those files in their new location.

Jan 3 2018, 10:29 AM

Jan 2 2018

beanz added a comment to D41660: [cmake] Add new linux toolchain file.

You should split the CMake cache file you created into two files, (1) a CMake Cache to manage the build configuration and (2) a tool chain file for targeting Linux. As @semeenai pointed out we absolutly want the behavior of the toolchain file being loaded multiple times. That is the correct way this build should work.

Jan 2 2018, 8:58 AM

Dec 19 2017

beanz accepted D41010: [orc][cmake] Check if 8 byte atomics require libatomic for unittest.

LGTM

Dec 19 2017, 9:41 AM

Dec 18 2017

beanz added a comment to D41010: [orc][cmake] Check if 8 byte atomics require libatomic for unittest.

I'm going to nitpick here a little. The patch is fine, but one of two things should change. Either line 29 should use list(APPEND...) instead of set, or you should do something more like:

Dec 18 2017, 4:44 PM
beanz added a comment to D41272: Don't try to run MCJIT/OrcJIT EH tests when C++ library is statically linked.

A few inline comments. Overall I like the direction.

Dec 18 2017, 4:39 PM
beanz added a comment to D37631: [libFuzzer] Support using libc++.

@phosek not sure if there is an equivalent. Probably strip -x is the closest.

Dec 18 2017, 3:15 PM
beanz added a comment to D37631: [libFuzzer] Support using libc++.

@phosek On darwin ld64 supports the flags -all_load and -force_load which work similarly to --whole-archive. The ld64 manpage has the documentation for those options.

Dec 18 2017, 10:09 AM

Dec 14 2017

beanz accepted D41244: Added a separate install target for compilert-rt-headers.

This patch LGTM.

Dec 14 2017, 9:42 AM

Dec 11 2017

beanz updated the diff for D38856: [IPSCCP] Remove calls without side effects.

Abstracting check for whether or not it is safe to remove an instruction as per the feedback from davide.

Dec 11 2017, 11:38 AM

Dec 8 2017

beanz accepted D40934: [cmake] Only pass CMAKE_SYSROOT if non-empty.

LGTM

Dec 8 2017, 11:32 AM

Dec 6 2017

beanz accepted D40818: [libcxxabi] Pass LIBCXXABI_SYSROOT and LIBCXXABI_GCC_TOOLCHAIN to lit.

LGTM

Dec 6 2017, 2:33 PM
beanz accepted D40815: [libcxxabi] Use the correct variable name for target triple in lit.

LGTM.

Dec 6 2017, 2:32 PM
beanz accepted D40814: [libcxx] Use the correct variable name for target triple in lit.

LGTM.

Dec 6 2017, 2:32 PM
beanz accepted D40637: [CMake] Support runtimes and monorepo layouts when looking for libc++.

LGTM

Dec 6 2017, 2:31 PM

Dec 4 2017

beanz accepted D40744: [cmake] Modernize some conditionals. NFC.

This is a workaround predating CMake 3.1 when they fixed the underlying issues with the expansion of variables in STREQUAL. See: https://cmake.org/cmake/help/v3.1/policy/CMP0054.html.

Dec 4 2017, 4:16 PM
beanz accepted D40761: [CMake] Don't use comma as an alternate separator.

LGTM.

Dec 4 2017, 4:09 PM
beanz accepted D40762: [CMake] Don't use comma as an alternate separator.
Dec 4 2017, 4:08 PM

Dec 1 2017

beanz added a reviewer for D40689: [llvm] Add install-distribution-stripped: phosek.
Dec 1 2017, 3:03 PM
beanz accepted D40740: [compiler-rt] Remove out of date comment.

LGTM

Dec 1 2017, 11:01 AM
beanz accepted D40687: [compiler-rt] Add install-*-stripped targets.

LGTM.

Dec 1 2017, 11:01 AM
beanz added a comment to D40687: [compiler-rt] Add install-*-stripped targets.

Yes, that comment is very out of date. The compiler-rt standalone build is essential to many of the users. It is quite common to build and use compiler-rt without LLVM (like sanitizer support in gcc).

Dec 1 2017, 10:53 AM
beanz accepted D40734: New users should be using the runtimes dir, not projects.

The docs changes look good to me. I think if we want to deprecate llvm/projects for runtime libraries, we should have a discussion on llvm-dev & cfe-dev. I think too few people are using llvm/runtimes today as part of their regular builds for me to feel comfortable deprecating llvm/projects without giving some notice first and getting more people to try the runtimes directory.

Dec 1 2017, 9:54 AM
beanz added inline comments to D40689: [llvm] Add install-distribution-stripped.
Dec 1 2017, 9:50 AM
beanz added a comment to D40687: [compiler-rt] Add install-*-stripped targets.

This change isn't safe. Compiler-RT is buildable without LLVM's modules as long as you disable the tests, so you can't use an AddLLVM function inside AddCompilerRT unless it is only used when tests are disabled.

Dec 1 2017, 9:49 AM

Nov 30 2017

beanz accepted D40685: [libunwind] Switch to add_llvm_install_targets.

LGTM.

Nov 30 2017, 3:12 PM
beanz accepted D40681: [libc++abi] Add install-cxxabi-stripped target.

LGTM.

Nov 30 2017, 3:10 PM
beanz accepted D40675: [clang] Use add_llvm_install_targets.

LGTM.

Nov 30 2017, 2:27 PM
beanz accepted D40620: [llvm] Add stripped installation targets.

LGTM. It is a little strange that CMake generates the install/strip target automatically, but won't let you put / in a target name, but there really isn't anything to be done about it.

Nov 30 2017, 1:20 PM
beanz accepted D40656: [cmake] Include project name in Sphinx doctree dir to fix race conditions.

LGTM.

Nov 30 2017, 10:52 AM
beanz accepted D40655: [cmake] Enable zlib support on windows.

LGTM!

Nov 30 2017, 10:51 AM

Nov 29 2017

beanz added a comment to D40620: [llvm] Add stripped installation targets.

So, I wasn't actually familiar with how CMAKE_INSTALL_DO_STRIP worked, so I went and researched it. I think we actually should mimic how that works. So instead of having an option that enables stripping on the install targets, we should create two targets, one that strips and one that doesn't. Then the user can select whether or not they want their install contents stripped by choosing which target they run.

Nov 29 2017, 3:50 PM
beanz added a comment to D40620: [llvm] Add stripped installation targets.

Is there a reason to add a new option instead of just adding -DCMAKE_INSTALL_DO_STRIP=${CMAKE_INSTALL_DO_STRIP} to the install commands?

Nov 29 2017, 2:59 PM
beanz accepted D40229: [cmake] Remove redundant call to cmake when building host tools..

Seems reasonable to me.

Nov 29 2017, 2:56 PM

Nov 27 2017

beanz accepted D40459: [cmake] Pass -Wl,-z,nodelete on Linux to prevent unloading.

LGTM.

Nov 27 2017, 2:22 PM
beanz accepted D40515: [CMake] Pass LLVM_HOST_TRIPLE to external projects .

LGTM.

Nov 27 2017, 1:06 PM
beanz accepted D40257: [CMake] Use LIST_SEPARATOR rather than escaping in ExternalProject_Add.

LGTM!

Nov 27 2017, 11:15 AM
beanz accepted D40202: Add opt-viewer testing.

LGTM!

Nov 27 2017, 10:37 AM
beanz added a comment to D39716: Explicitly set CMake policy CMP0068 to NEW to avoid warnings.

If we're setting the policy to NEW we should also be setting BUILD_WITH_INSTALL_NAME_DIR to match BUILD_WITH_INSTALL_RPATH, otherwise we're making builds using CMake 3.9 and later build differently than earlier versions of CMake which is undesirable.

Nov 27 2017, 10:34 AM
beanz accepted D40459: [cmake] Pass -Wl,-z,nodelete on Linux to prevent unloading.

LGTM

Nov 27 2017, 10:26 AM
beanz added a comment to D40326: Fix static link on debian.

I don't see anything wrong with this patch as a workaround, but it seems like there might be a bigger issue here.

Nov 27 2017, 10:16 AM