beanz (Chris Bieneman)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Fri, Nov 17

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

LGTM.

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

LGTM.

Fri, Nov 17, 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.

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

One small nitpick, otherwise LGTM!

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

LGTM

Fri, Nov 17, 9:20 AM

Wed, Nov 15

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.

Wed, Nov 15, 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.

Wed, Nov 15, 1:50 PM
beanz added a comment to D39939: [cmake] Allow appending a free-form 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.

Wed, Nov 15, 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.

Wed, Nov 15, 11:50 AM

Tue, Nov 14

beanz added a comment to D39939: [cmake] Allow appending a free-form 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.

Tue, Nov 14, 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.

Tue, Nov 14, 1:50 PM

Mon, Nov 13

beanz accepted D37644: [CMake][runtimes] Use list of lists rather than ":" delimiters.

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

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

Looks reasonable to me

Mon, Nov 13, 2:41 PM
beanz added a comment to D39939: [cmake] Allow appending a free-form 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.

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

LGTM.

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

LGTM.

Mon, Nov 13, 2:13 PM

Wed, Nov 8

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.

Wed, Nov 8, 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.

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

One comment inline below.

Wed, Nov 8, 9:25 AM

Tue, Nov 7

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.

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

LGTM.

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

LGTM.

Tue, Nov 7, 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.

Tue, Nov 7, 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?

Tue, Nov 7, 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
beanz added inline comments to D35648: [CMake] Add `project` in `runtime/CMakeList.txt`.
Aug 29 2017, 3:13 PM

Aug 28 2017

beanz added a comment to D37027: Fix cmake check for futimens when deploying to earlier macOS releases..

LGTM.

Aug 28 2017, 3:09 PM
beanz accepted D37027: Fix cmake check for futimens when deploying to earlier macOS releases..
Aug 28 2017, 3:09 PM
beanz added inline comments to D35648: [CMake] Add `project` in `runtime/CMakeList.txt`.
Aug 28 2017, 9:45 AM
beanz accepted D35133: [cmake] Use new GetSVN.cmake SOURCE_DIRS parameter.

Since the dependent patch landed, this LGTM too.

Aug 28 2017, 9:41 AM

Aug 22 2017

beanz added inline comments to D35648: [CMake] Add `project` in `runtime/CMakeList.txt`.
Aug 22 2017, 3:54 PM

Aug 10 2017

beanz accepted D36541: [CMake] Include LLVMFuzzer in Fuchsia toolchain.

LGTM!

Aug 10 2017, 5:14 PM
beanz accepted D36540: [CMake] Add install target for LLVMFuzzer.

LGTM!

Aug 10 2017, 5:13 PM
beanz accepted D36349: [CMake] Build sanitized C++ runtimes for Fuchsia.

This also looks good to me.

Aug 10 2017, 5:05 PM
beanz accepted D36348: [CMake][runtimes] Support for building target variants.

This looks pretty cool to me.

Aug 10 2017, 5:03 PM

Aug 9 2017

beanz accepted D35077: [RFC] Build LLVM-C.dll on MSVC that exports only the C API.

LGTM.

Aug 9 2017, 10:26 AM

Aug 4 2017

beanz added a comment to D31367: Expression: add missing linkage to RuntimeDyld component.

@mgorny I want to apologize here but I need to ask you not to commit this until @chapuni and I can come to an agreement between this solution and D36211. He ping'd me on IRC yesterday after I left for the day, I'll try and get in touch with him today to see if we can agree on the path forward.

Aug 4 2017, 9:56 AM · Restricted Project

Aug 3 2017

beanz added a comment to D31367: Expression: add missing linkage to RuntimeDyld component.

@chapuni, I disagree. LLDBExpression doesn't actually use RuntimeDyld directly, rather ExecutionEngine's headers do. I don't want downstream users of ExecutionEngine to know about the things it neccesitates pulling in.

Aug 3 2017, 2:05 PM · Restricted Project
beanz accepted D36211: [cmake] Expose the dependencies of ExecutionEngine as PUBLIC.
Aug 3 2017, 1:56 PM
beanz added a comment to D36211: [cmake] Expose the dependencies of ExecutionEngine as PUBLIC.

@chapuni, the reason that I wanted to have this done with PUBLIC interface specifications is because lldbExpression does't actually use anything from the RuntimeDYLD library. The problem is LLVMExecutionEngine's headers are not clean, and they reference RuntimeDYLD directly and result in needing to bring it in.

Aug 3 2017, 1:56 PM
beanz accepted D30155: [clang-tools-extra] [test] Fix clang library dir in LD_LIBRARY_PATH For stand-alone build.

Looks reasonable to me.

Aug 3 2017, 11:17 AM · Restricted Project
beanz accepted D35311: lldb-server tests: Add support for testing debugserver.

This all looks fine to me. The one thing I'd like you to consider is that someday (when lldb-server is supported on Darwin) we will want to run these tests against both debugserver and lldb-server. I'm not asking you to make those changes to this patch, but if there are things you would change about how you're doing things to make that easier to support I'd ask that you consider that.

Aug 3 2017, 10:57 AM

Jul 26 2017

beanz accepted D35924: [CMake] Disable -Werror for CMake checks.

LGTM!

Jul 26 2017, 4:49 PM

Jul 18 2017

beanz accepted D35219: Generate a separate compile_commands.json DB for external projects.

LGTM!

Jul 18 2017, 2:00 PM
beanz added inline comments to D33048: [CMake] runtimes test targets need to depend on LLVM tools.
Jul 18 2017, 1:46 PM
beanz added inline comments to D33048: [CMake] runtimes test targets need to depend on LLVM tools.
Jul 18 2017, 1:44 PM
beanz added a comment to D35077: [RFC] Build LLVM-C.dll on MSVC that exports only the C API.

I don't think this should be added as a new tool. We support generating LLVM-C.dylib on Darwin via the tools/llvm-shlib tool. It would be better if this were added there instead of completely separate.

Jul 18 2017, 1:41 PM
beanz accepted D34375: [libunwind][CMake] Set library dir to be LLVM's intermediate output dir.

LGTM!

Jul 18 2017, 1:38 PM
beanz added a comment to D35346: [CMake] Enable buildings builtins for Darwin as part of runtimes.

So... Looking at the CMake in compiler-rt, COMPILER_RT_DEFAULT_TARGET_ONLY is only interpreted in a NOT APPLE block, so this would really not behave the way you intend. I think it is better to not support this on Darwin for the time being and provide a hard error rather than behaving incorrectly.

Jul 18 2017, 1:35 PM
beanz added a comment to D35346: [CMake] Enable buildings builtins for Darwin as part of runtimes.

I'm suspicious that this would work as intended. What builtin libraries get built if your triple is Darwin?

Jul 18 2017, 1:31 PM
beanz accepted D35343: [CMake] Set toolchain tools in cross-target runtimes build.

LGTM

Jul 18 2017, 1:29 PM

Jul 10 2017

beanz accepted D33760: [libunwind][CMake] Add install path variable to allow overriding the destination.

LGTM!

Jul 10 2017, 9:22 AM
beanz accepted D33762: [libcxx][CMake] Add install path variable to allow overriding the destination.

This LGTM.

Jul 10 2017, 9:22 AM
beanz accepted D33761: [libcxxabi][CMake] Add install path variable to allow overriding the destination.

LGTM

Jul 10 2017, 9:20 AM
beanz accepted D33048: [CMake] runtimes test targets need to depend on LLVM tools.

LGTM.

Jul 10 2017, 9:19 AM
beanz accepted D35080: [Docs] Updating CMake docs to include LLVM_REVERSE_ITERATION.

LGTM!

Jul 10 2017, 8:33 AM
beanz accepted D35024: Remove obsolete "unset" in CMake configuration.

LGTM! Thanks for the cleanup!

Jul 10 2017, 8:16 AM
beanz accepted D35023: Removes obsolete documentation section.

LGTM! Thanks for updating the docs. I am really happy to be able to purge that broken behavior from my mind!

Jul 10 2017, 8:14 AM
beanz accepted D35017: [CMake] Use tools template for clangd and modularize.

LGTM!

Jul 10 2017, 8:10 AM

Jul 9 2017

beanz accepted D34520: Minor correctness improvements for LLVMTestingSupport.

LGTM!

Jul 9 2017, 6:13 AM
beanz added a comment to D27701: [lit] Fix discovery test on Windows.

I think it is better to XFAIL Windows and have test coverage on non-windows systems than to have the tests not running anywhere.

Jul 9 2017, 6:13 AM

Jul 7 2017

beanz added inline comments to D33760: [libunwind][CMake] Add install path variable to allow overriding the destination.
Jul 7 2017, 10:10 AM

Jun 21 2017

beanz accepted D33707: TableGen.cmake: Use DEPFILE for Ninja Generator with CMake>=3.7..

This looks great! Thanks for doing this.

Jun 21 2017, 3:00 PM

Jun 19 2017

beanz accepted D32816: [CMake] Support multi-target runtimes build.

CMake stuff all looks good to me!

Jun 19 2017, 11:48 AM
beanz added a comment to D34322: [lldb] Correctly escape newlines and backslashes in the JSON serializer.

I get your perspective, but holding up this relatively small patch that fixes a bug in existing code on an architectural disagreement seems like excessive bike shedding. If we assume that JSON is required for the use case would you have Kuba write a full JSON parser in LLVM to satisfy your distaste over the fact that we have multiple JSON parsers already? That seems like an unreasonable request just to fix a few small bugs in existing code.

Jun 19 2017, 9:39 AM · Restricted Project
beanz added a comment to D34322: [lldb] Correctly escape newlines and backslashes in the JSON serializer.

My suggestion would be to just use the YAML writer for now and leave a comment to make it clear that this is really JSON only.

Jun 19 2017, 9:25 AM · Restricted Project
beanz added inline comments to D32835: [compiler-rt] [cmake] Support generic installation.
Jun 19 2017, 9:14 AM
beanz added a comment to D32816: [CMake] Support multi-target runtimes build.

One question inline below.

Jun 19 2017, 9:06 AM

Jun 16 2017

beanz accepted D34282: Call cmake_minimum_required at the top of CMakeLists.txt.

LGTM

Jun 16 2017, 1:23 PM
beanz accepted D33662: [CMake] Introduce LLVM_TARGET_TRIPLE_ENV as an option to override LLVM_DEFAULT_TARGET_TRIPLE at runtime..

LGTM

Jun 16 2017, 1:19 PM

Jun 12 2017

beanz added inline comments to D32816: [CMake] Support multi-target runtimes build.
Jun 12 2017, 4:43 PM

May 31 2017

beanz added a comment to D22499: [llvm-config] Report --bindir based on LLVM_TOOLS_INSTALL_DIR.

That looks like that should work to me. Seems good. @tstellar, does this resolve your issue?

May 31 2017, 3:45 PM
beanz accepted D32595: CMakeLists: Don't set LLVM_MAIN_SRC_DIR when building stand-alone clang.

Ah. I see what you're saying. I was thinking of the mismatch in a different way, but your solution makes sense. LGTM.

May 31 2017, 3:25 PM
beanz accepted D32668: CMake: Split static library exports into their own export file.

Ah. Makes sense. Can we maybe change the name from "StaticExports" to "ComponentExports"? As @chapuni pointed out this really applies to the component libraries, which can be static or shared based on configuration (although I keep hoping nobody would ever use BUILD_SHARED_LIBS in a distribution). Otherwise, I think this patch is good.

May 31 2017, 3:20 PM
beanz added a comment to D33444: Fix an issue of creating symlinks when llvm is embedded.

Unfortunately that doesn't really explain the need for these patches. From searching GitHub it doesn't seem like your project is overriding CMAKE_CFG_INTDIR. What is your CMake command line?

May 31 2017, 1:52 PM
beanz added a comment to D22499: [llvm-config] Report --bindir based on LLVM_TOOLS_INSTALL_DIR.

Ah! I see now. Probably the only thing that needs to be changed is handling LLVM_TOOLS_INSTALL_DIR being absolute. Otherwise the patch seems good to me.

May 31 2017, 1:47 PM
beanz added a comment to D33444: Fix an issue of creating symlinks when llvm is embedded.

How are you configuring? Unless I'm misunderstanding something when CMAKE_CONFIGURATION_TYPES is set CMAKE_CFG_INTDIR will never be ".".

May 31 2017, 11:44 AM
beanz added a comment to D22499: [llvm-config] Report --bindir based on LLVM_TOOLS_INSTALL_DIR.

I'm confused. In what situation is llvm-config installed into a directory that isn't the LLVM_TOOLS_INSTALL_DIR? It seems to me that if llvm-config is installed in LLVM_TOOLS_INSTALL_DIR this should work.

May 31 2017, 11:39 AM
beanz added inline comments to D32835: [compiler-rt] [cmake] Support generic installation.
May 31 2017, 11:31 AM
beanz accepted D32817: [CMake] Build runtimes for Fuchsia targets.

This all looks great. I'm really excited to have such a complete toolchain build example in-tree. Thank you!

May 31 2017, 11:24 AM
beanz accepted D31687: CMake: Fix sphinx build with standalone clang.

LGTM.

May 31 2017, 11:23 AM
beanz added a reviewer for D32734: [CMake][runtimes] Set default directory for runtime libraries: phosek.

Sorry for the delay, I've been out of town. I want to loop @phosek into this. I don't think this patch is correct. When I build on Darwin the sanitizer libraries are put in the build directory under lib/clang/$version/darwin/lib (which is correct), and the other runtimes are built under /lib/. This all seems right to me, so I don't understand the problem you're experiencing.

May 31 2017, 11:22 AM
beanz added a comment to D32668: CMake: Split static library exports into their own export file.

I want to echo @chapuni's question. What is the use case where a distribution would install the CMake modules and not install the LLVM component libraries? It doesn't seem to me that there is any reason to install the CMake modules to support find_package without also installing the static archives.

May 31 2017, 11:19 AM
beanz accepted D32710: [CMake][runtimes] Add install target for runtimes builtins.

LGTM!

May 31 2017, 11:15 AM