Make `llvm-config` detect whether or not LLVM is a shared library. If it is, then only try to link …

Make `llvm-config` detect whether or not LLVM is a shared library. If it is, then only try to link to the shared lib.

R= jfb@chromium.org
BUG=

[JF, 2015-06-24 17:12:53]

Could you explain what this does in more details? Or rather: what isn't working?

I'm not familiar with the configuration bits here, so I'm mildly surprised that
it wouldn't already work!

Also, this seems like it could go straight to upstream LLVM?

[Richard Diamond, 2015-06-24 18:42:58]

On 2015/06/24 at 17:12:53, jfb wrote:
> Could you explain what this does in more details? Or rather: what isn't
working?

Sure, I'll describe it here and put describe in greater detail in the commit
message.

Currently, the PNaCl SDK LLVM is a single shared object, ie
`lib/libLLVM-3.7.0svn.so`, and thus isn't separated into its constituent
components.

I've recently discovered (after I published this patch), however, that LLVM's
CMake build system && its `autoconfig` build system actually differ on this
detail. CMake creates the separate shared objects, as `llvm-config` expects, but
when one uses `autoconfig` (setting `--enable-shared --disable-static` verily),
`make` creates static libraries and then links these together to create a single
`.so`. Not hard to fix, but it raises a question: should the Makefile build
system be fixed (the 'better' fix), or should I just fix it in `llvm-config`
(easier)? I'm leaning toward the latter, considering I've heard people mention
that the Makefile build system is somewhat deprecated anyway.

> I'm not familiar with the configuration bits here, so I'm mildly surprised
that it wouldn't already work!
> 
> Also, this seems like it could go straight to upstream LLVM?

Agreed and I had planned on upstreaming, however, as I've experienced in Rust,
more dbg info APIs have changed since 3.7, so you'd have to also merge those
(plus the clang changes). I decided to land here first so it could land in
`pepper_canary` as soon as possible, as the Rust+PNaCl work can't build without
this change or without modifying a auto generated file, though in some ways I
can see how short sighted my choice was.

[JF, 2015-06-24 21:09:02]

Could you also test this change with the cmake build, to make sure it doesn't
break it?

I think this can go straight to upstream with the suggested changes, and then we
can cherry-pick to our repo so it's available ASAP.

https://codereview.chromium.org/1196003003/diff/30001/tools/llvm-config/llvm-...
File tools/llvm-config/llvm-config.cpp (right):

https://codereview.chromium.org/1196003003/diff/30001/tools/llvm-config/llvm-...
tools/llvm-config/llvm-config.cpp:354: const bool IsSharedLib =
(std::strcmp(BUILD_SHARED_LIBS, "ON") == 0);
Could you change BUILD_SHARED_LIBS's name? There's a similarly-named variable in
cmake.

https://codereview.chromium.org/1196003003/diff/30001/tools/llvm-config/llvm-...
tools/llvm-config/llvm-config.cpp:357: RequiredLibs.push_back("libLLVM-"
PACKAGE_VERSION ".so");
Here and on line 375 you hard-code ".so". That seems fine since the code already
hard-codes ".a", but you should add ".dylib" for Mac (I'm assuming that's what
LLVm is configured to generate).

https://codereview.chromium.org/1196003003/diff/30001/tools/llvm-config/llvm-...
tools/llvm-config/llvm-config.cpp:375: } else if (Lib.endswith(".so")) {
Here too.

[Richard Diamond, 2015-06-24 21:19:16]

On 2015/06/24 at 21:09:02, jfb wrote:
> Could you also test this change with the cmake build, to make sure it doesn't
break it?

It doesn't break LLVM's CMake build (I've been using CMake because Ice Cream
(https://github.com/icecc/icecream) actually works with it). But I think that's
just a result of the fact that the LLVM build system doesn't use `llvm-config`.

Either way, now that I know that there is a difference between CMake &&
autoconfig, I need to fix `llvm-config` to Do The Right Thing. I haven't fixed
it yet because I realized I've found a few untested corner cases in
https://codereview.chromium.org/1207543003 and am currently in the process of
fixing the new bugs.

> I think this can go straight to upstream with the suggested changes, and then
we can cherry-pick to our repo so it's available ASAP.

Alright, will do.

[Richard Diamond, 2015-06-24 21:44:30]

https://codereview.chromium.org/1196003003/diff/30001/tools/llvm-config/llvm-...
File tools/llvm-config/llvm-config.cpp (right):

https://codereview.chromium.org/1196003003/diff/30001/tools/llvm-config/llvm-...
tools/llvm-config/llvm-config.cpp:354: const bool IsSharedLib =
(std::strcmp(BUILD_SHARED_LIBS, "ON") == 0);
On 2015/06/24 at 21:09:01, JF wrote:
> Could you change BUILD_SHARED_LIBS's name? There's a similarly-named variable
in cmake.

`BUILD_SHARED_LIBS` is the CMake var name. Or did you want me to change the name
to something that's not a CMake var name?

[JF, 2015-06-24 21:52:43]

https://codereview.chromium.org/1196003003/diff/30001/tools/llvm-config/llvm-...
File tools/llvm-config/llvm-config.cpp (right):

https://codereview.chromium.org/1196003003/diff/30001/tools/llvm-config/llvm-...
tools/llvm-config/llvm-config.cpp:354: const bool IsSharedLib =
(std::strcmp(BUILD_SHARED_LIBS, "ON") == 0);
On 2015/06/24 21:44:30, Richard Diamond wrote:
> On 2015/06/24 at 21:09:01, JF wrote:
> > Could you change BUILD_SHARED_LIBS's name? There's a similarly-named
variable
> in cmake.
> 
> `BUILD_SHARED_LIBS` is the CMake var name. Or did you want me to change the
name
> to something that's not a CMake var name?

If it's named the same as cmake's but doesn't have anything to do with it then
it'll be pretty confusing, so it should have a different name.

[Richard Diamond, 2015-06-24 21:54:48]

https://codereview.chromium.org/1196003003/diff/30001/tools/llvm-config/llvm-...
File tools/llvm-config/llvm-config.cpp (right):

https://codereview.chromium.org/1196003003/diff/30001/tools/llvm-config/llvm-...
tools/llvm-config/llvm-config.cpp:354: const bool IsSharedLib =
(std::strcmp(BUILD_SHARED_LIBS, "ON") == 0);
On 2015/06/24 at 21:52:42, JF wrote:
> On 2015/06/24 21:44:30, Richard Diamond wrote:
> > On 2015/06/24 at 21:09:01, JF wrote:
> > > Could you change BUILD_SHARED_LIBS's name? There's a similarly-named
variable
> > in cmake.
> > 
> > `BUILD_SHARED_LIBS` is the CMake var name. Or did you want me to change the
name
> > to something that's not a CMake var name?
> 
> If it's named the same as cmake's but doesn't have anything to do with it then
it'll be pretty confusing, so it should have a different name.

Setting `BUILD_SHARED_LIBS` to `ON` tells LLVM to build shared libraries, so
it's apropos.

[Richard Diamond, 2015-06-24 22:31:49]

I've addressed your comments JF; I'm now going to continue development upstream.

[Richard Diamond, 2015-06-24 22:54:30]

On 2015/06/24 at 22:31:49, Richard Diamond wrote:
> I've addressed your comments JF; I'm now going to continue development
upstream.

Upstream LLVM patch: http://reviews.llvm.org/D10716