beanz (Chris Bieneman)
User

Projects

User does not belong to any projects.

User Details

User Since
Aug 20 2014, 4:35 PM (196 w, 3 d)

Recent Activity

Fri, May 18

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

Looping in @bruno in case he has feedback.

Fri, May 18, 9:57 AM

Thu, May 17

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.

Thu, May 17, 9:52 AM

Thu, May 10

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

Tue, May 1

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.

Tue, May 1, 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
beanz accepted D40219: [CMake] Add LLVM_ENABLE_IDE option to better process sources for IDE's.

One comment below. Otherwise LGTM.

Nov 27 2017, 10:10 AM
beanz accepted D40233: [CMake][runtimes] Support monorepo layout with runtimes build.

LGTM.

Nov 27 2017, 9:55 AM
beanz accepted D40280: [CMake][libcxx] Include AddLLVM needed for tests in the right context.

LGTM.

Nov 27 2017, 9:53 AM
beanz accepted D40258: [CMake] Support side-by-side checkouts in multi-stage build.

LGTM.

Nov 27 2017, 9:52 AM
beanz accepted D40232: [CMake] Use LIST_SEPARATOR rather than escaping in ExternalProject_Add.

LGTM.

Nov 27 2017, 9:52 AM
beanz accepted D39734: [cmake] Pass LLVM_USE_LINKER flag to CrossCompile toolchain if set.

LGTM.

Nov 27 2017, 9:51 AM
beanz accepted D40229: [cmake] Remove redundant call to cmake when building host tools..

LGTM.

Nov 27 2017, 9:50 AM
beanz added a comment to D39930: [CMake] Use libc++ and compiler-rt as default libraries in Fuchsia toolchain.

@phosek, I would love to see compiler-rt fully refactored so that we target one triple at a time always. I'd also love to see the clang driver refactored so that the code to resolve paths to runtime libraries was shared so we follow the same conventions across all driver implementations.

Nov 27 2017, 9:49 AM
beanz added inline comments to D40202: Add opt-viewer testing.
Nov 27 2017, 9:44 AM

Nov 17 2017

beanz accepted D40195: [libunwind][CMake] Provide option to disable instalation of the library.

LGTM.

Nov 17 2017, 1:46 PM
beanz accepted D40194: [libcxxabi][CMake] Provide option to disable installing of the library.

LGTM.

Nov 17 2017, 1:46 PM
beanz added a comment to D39949: [CMake][libcxxabi] Support merging archives when statically linking unwinder.

@mstorsjo that is the problem that the LLVM/runtimes directory solves. It supports building all the runtime libraries as a single build with one build-tree per target and the management of separate build trees is handled by the build system.

Nov 17 2017, 1:30 PM
beanz accepted D39949: [CMake][libcxxabi] Support merging archives when statically linking unwinder.

One small nitpick, otherwise LGTM!

Nov 17 2017, 9:25 AM
beanz accepted D39988: [CMake][runtimes] Don't passthrough prefixes for non-default targets.

LGTM

Nov 17 2017, 9:20 AM

Nov 15 2017

beanz accepted D40087: [CMake][runtimes] Use cmake_parse_arguments in runtimes functions.

Generally we frown on commit messages as concise as your description here. Might be worth a quick sentence or two explaining why cmake_parse_aguments is better as a breadcrumb for anyone who comes along in the future.

Nov 15 2017, 2:06 PM
beanz added a comment to D39734: [cmake] Pass LLVM_USE_LINKER flag to CrossCompile toolchain if set.

The code path you're modifying isn't just for building optimized tools, it also gets triggered when building the host tools in cross compilations. It is totally reasonable that I might want to use a different linker for host binaries from the linker used for target binaries. One example would be if I'm cross compiling to say PPC and wanting to use gold, but want to use lld as the linker for the host tools.

Nov 15 2017, 1:50 PM
beanz added a comment to D39939: [cmake] Append LLVM_VERSION_SUFFIX to SOVERSION.

I think leaving VERSION as it is today, but changing SOVERSION to be ${LLVM_VERSION_MAJOR}${LLVM_VERSION_SUFFIX} makes the most sense to me. That should meet your needs.

Nov 15 2017, 1:43 PM
beanz added a comment to D39949: [CMake][libcxxabi] Support merging archives when statically linking unwinder.

Rather than hand rolling ar commands, could we make libunwind, libcxxabi, and libcxx produce object libraries, then we could just create normal static library targets with the object libraries as sources.

Nov 15 2017, 11:50 AM

Nov 14 2017

beanz added a comment to D39939: [cmake] Append LLVM_VERSION_SUFFIX to SOVERSION.

Ok. Rather than adding a new option I think you should just add LLVM_VERSION_SUFFIX to the SOVERSION property, then that should work for you.

Nov 14 2017, 2:15 PM
beanz added a comment to D39734: [cmake] Pass LLVM_USE_LINKER flag to CrossCompile toolchain if set.

I don't think we can do this unconditionally like this. It seems to me like you could have a situation where you want to use different linkers for the host tools and target binaries.

Nov 14 2017, 1:50 PM

Nov 13 2017

beanz accepted D37644: [CMake][runtimes] Use variables rather than ":" delimiters.

Sorry for the long delay on this review. It keeps getting lost in my inbox.

Nov 13 2017, 3:58 PM
beanz accepted D39938: [compiler-rt] [builtins] Include GENERIC_SOURCES in arm_SOURCES for MinGW.

Looks reasonable to me

Nov 13 2017, 2:41 PM
beanz added a comment to D39939: [cmake] Append LLVM_VERSION_SUFFIX to SOVERSION.

Who is the "we" you mention in your commit message? This seems to me like a patch very specific to a single downstream user, and you describe it as temporary, so I'm not convinced we should take it upstream.

Nov 13 2017, 2:19 PM
beanz accepted D39715: [CMake][runtimes] Set compiler as working even for default target.

LGTM.

Nov 13 2017, 2:15 PM
beanz accepted D39932: [CMake][runtimes] Don't process common options in runtimes build .

LGTM.

Nov 13 2017, 2:13 PM

Nov 8 2017

beanz accepted D39811: [CMake][runtimes] Fix the variable name.

LGTM. In the future you can just commit this kind of patch without review. It definitely falls under the category of obvious good fix.

Nov 8 2017, 3:07 PM
beanz accepted D39029: [CMake] Passthrough CMAKE_SYSROOT to external projects.

Sounds reasonable to me. The added complexity of not passing it doesn't seem worth it.

Nov 8 2017, 10:11 AM
beanz added a comment to D39715: [CMake][runtimes] Set compiler as working even for default target.

One comment inline below.

Nov 8 2017, 9:25 AM

Nov 7 2017

beanz added a comment to D39029: [CMake] Passthrough CMAKE_SYSROOT to external projects.

Sorry I've been bad at staying on top of your patches.

Nov 7 2017, 3:32 PM
beanz accepted D38471: Allow compiler-rt test targets to work with multi-config CMake generators.

LGTM.

Nov 7 2017, 3:22 PM
beanz accepted D39299: [CMake] Remove target to build native tablegen.

LGTM.

Nov 7 2017, 3:16 PM
beanz accepted D39298: [CMake] Add custom target to create build directory.

LGTM. I keep meaning to spend time rewriting all this to use the LLVMExternalProjectUtils module which avoids issues like this, but I never really have the time to do it.

Nov 7 2017, 3:14 PM
beanz accepted D39041: [Support] Remove an outdated comment..

LGTM. I am curious what your use case for adding and removing options is, can you elaborate?

Nov 7 2017, 2:18 PM

Oct 17 2017

beanz accepted D39002: [cmake] Use find_package to discover zlib.

LGTM

Oct 17 2017, 1:22 PM
beanz requested changes to D39002: [cmake] Use find_package to discover zlib.
Oct 17 2017, 11:02 AM

Oct 12 2017

beanz added inline comments to D38856: [IPSCCP] Remove calls without side effects.
Oct 12 2017, 5:39 PM
beanz created D38856: [IPSCCP] Remove calls without side effects.
Oct 12 2017, 10:47 AM

Oct 5 2017

beanz added a comment to D31363: [libc++] Remove cmake glob for source files.

Building libcxx without LLVM's CMake modules is very important, not just to Apple. This is how several open source distributions work, and it would be a huge disservice to break this. Same is true for compiler-rt, libcxxabi, libunwind, etc.

Oct 5 2017, 9:20 AM

Oct 2 2017

beanz added a comment to D38471: Allow compiler-rt test targets to work with multi-config CMake generators.

You can put the include inside the function body of configure_compiler_rt_lit_site_cfg, but you can't put it just inside the file. By design AddCompilerRT is mostly usable without AddLLVM.

Oct 2 2017, 4:57 PM
beanz added inline comments to D38471: Allow compiler-rt test targets to work with multi-config CMake generators.
Oct 2 2017, 2:24 PM

Sep 29 2017

beanz accepted D38389: [CMake] Remove `CMAKE_.*_OUTPUT_DIRECTORY` (NFCI).

LGTM. Please watch the bots closely with this change because CMake can be used in strange ways.

Sep 29 2017, 11:10 AM

Sep 27 2017

beanz accepted D38317: [CMake] Fix typo: "in-tree" -> "in-source" (NFC).

LGTM!

Sep 27 2017, 11:27 AM

Sep 19 2017

beanz accepted D37859: [cmake] Add SOURCE_DIR argument to llvm_check_source_file_list.

LGTM!

Sep 19 2017, 10:33 AM

Sep 8 2017

beanz added a comment to D37414: [CMake] Add bootstrap option `CLANG_BOOTSTRAP_USE_LIBCXX`.

I'm not sure this change is sufficient to do what you're intending. I think you also need to pass the library path of the cxx library into the bootstrap configuration otherwise it will use the one from the sysroot.

Sep 8 2017, 1:38 PM
beanz accepted D37450: [CMake][runtimes] Use the same configuration for non-target and "default" target.

LGTM!

Sep 8 2017, 1:35 PM
beanz added inline comments to D37637: [CMake] Determine early on which projects are enabled.
Sep 8 2017, 1:33 PM
beanz added inline comments to D37602: Properly hook debuginfo-tests up to lit and CMake.
Sep 8 2017, 1:25 PM

Aug 31 2017

beanz accepted D36598: cmake + xcode: prevent gtests from using includes from project root.

This LGTM.

Aug 31 2017, 11:45 AM
beanz accepted D37245: [CMake][runtimes] Use target specific name for all runtimes targets.

As a note: at some point in the future I'd like us to get rid of the practice of using ":" to create lists of lists, instead using list of list names, which is a bit cleaner.

Aug 31 2017, 11:44 AM

Aug 29 2017

beanz accepted D35648: [CMake] Add `project` in `runtime/CMakeList.txt`.

LGTM.

Aug 29 2017, 3:14 PM