ldionne added inline comments.
================
Comment at: bolt/docs/OptimizingClang.md:228-236
$ CPATH=${TOPLEV}/stage1/install/bin/
-$ cmake -G Ninja ${TOPLEV}/llvm-project/llvm -DLLVM_TARGETS_TO_BUILD=X86 \
+$ cmake -G Ninja -S ${TOPLEV}/llvm-project/llvm -B ${TOPLEV}/stage2-prof-gen \
+ -DLLVM_TARGETS_TO_BUILD=X86 \
-DCMAKE_BUILD_TYPE=Release \
-DCMAKE_C_COMPILER=$CPATH/clang -DCMAKE_CXX_COMPILER=$CPATH/clang++ \
-DLLVM_ENABLE_PROJECTS="clang;lld" \
-DLLVM_USE_LINKER=lld -DLLVM_BUILD_INSTRUMENTED=ON \
----------------
Quuxplusone wrote:
> ldionne wrote:
> > Quuxplusone wrote:
> > > FWIW, I personally prefer to run cmake and ninja //from// the build
> > > directory, i.e.
> > > ```
> > > $ mkdir ${TOPLEV}/llvm-project/llvm/stage2-prof-gen
> > > $ cd ${TOPLEV}/llvm-project/llvm/stage2-prof-gen
> > > $ CPATH=${TOPLEV}/stage1/install/bin
> > > $ cmake -G Ninja \
> > > -DLLVM_TARGETS_TO_BUILD=X86 \
> > > ~~~
> > > -DCMAKE_INSTALL_PREFIX=${TOPLEV}/stage2-prof-gen/install \
> > > ../llvm-project/llvm
> > > $ ninja install
> > > ```
> > > This is the style that was used in `DataFlowSanitizer.rst` below, which
> > > you explicitly moved away from into `-B`/`-S`/`-C` land (where I find it
> > > harder to keep straight all the different extra options that are needed).
> > >
> > > Orthogonally, I'm worried that the advice on lines 73–74 of README.md
> > > doesn't mention `CMAKE_INSTALL_PREFIX`. When I'm building libc++ locally
> > > as part of my development workflow, I //absolutely do not// want to blow
> > > away my computer's default standard library installation; but what //do//
> > > I want to do? Should I use something like
> > > `-DCMAKE_INSTALL_PREFIX=$(pwd)/install` as depicted here? Can I really
> > > not get away with "running it out of the buildroot" the way I'm used to?—
> > > I //must// install it somewhere on my system in order to test it at all?
> > > Re: `-C` `-B` `-S`
> >
> > I really like using those flags because it makes the command relocatable,
> > i.e. it works regardless of where it is being run. As such, it's way more
> > copy-pasteable too. However, since that's orthogonal to this change, I'll
> > revert to the old style just so we have fewer things to argue about. But in
> > general, for libc++/libc++abi/libunwind instructions, you'll find that I
> > want things documented this way for that reason.
> >
> > > Re: `CMAKE_INSTALL_PREFIX`
> >
> > You don't necessarily need to install it in order to test it -- we test
> > libc++ all the time without running the `install-cxx` target. Furthermore,
> > I would expect that most systems nowadays have some sort of integrity
> > protection that prevents you from doing something bad like that. Apple
> > platforms certainly do -- you simply can't overwrite your `libc++.1.dylib`
> > anymore and render your system unusable. Still, I'll add a word of caution
> > to `CMAKE_INSTALL_PREFIX` above even though it's orthogonal to this patch
> > -- it's easy enough to do.
> FWIW, the `CMAKE_INSTALL_PREFIX` change is an improvement but only
> //slightly// more reassuring than before. ;) Part of my paranoia is that I
> happen to know that Homebrew stores things in `/usr/local/...` as well
> (although probably entirely under `/usr/local/Cellar/...`?) and I wouldn't
> want to blow those away //either//.
> Apparently on my Apple system I have `/usr/lib/libc++.dylib`,
> `/System/DriverKit/usr/lib/libc++.dylib`, and
> `/usr/local/Cellar/llvm/12.0.1/lib/libc++.dylib`.
>
> > we test libc++ all the time without running the `install-cxx` target
>
> Well, I know I do, but I don't use ENABLE_RUNTIMES yet... and the how-to
> below //does// end with the line `ninja -C build install-cxx`. If there's a
> supported/expected-to-work way to build and test libc++ that doesn't use
> `ninja install-cxx`, it'd be cool to mention it.
>
> Anyway, this is all just me fudding and shouldn't block this PR. :)
I think your comments are valid, but I think I would defer to @rafauler if they
want to change the Bolt documentation in that way.
> Well, I know I do, but I don't use ENABLE_RUNTIMES yet...
FWIW, `LLVM_ENABLE_RUNTIMES` won't change anything with respect to that.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119351/new/
https://reviews.llvm.org/D119351
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
