[PATCH] D118452: [polly][unittests] Link DeLICMTests with libLLVMCore

[Rainer Orth via Phabricator]

ro created this revision.
ro added a reviewer: beanz.
ro added a project: Polly.
Herald added subscribers: pengfei, asbirlea, fedor.sergeev, mgorny.
Herald added a reviewer: bollu.
ro requested review of this revision.
Herald added a project: LLVM.

A `-DBUILD_SHARED_LIBS=ON` build on Solaris/amd64 failed with

Undefined first referenced
symbol in file
_ZNK4llvm3cfg6UpdateIPNS_10BasicBlockEE4dumpEv tools/polly/unittests/DeLICM/CMakeFiles/DeLICMTests.dir/DeLICMTest.cpp.o (symbol belongs to implicit dependency /var/llvm/local-amd64-release-stage2-shared-A/bin/../lib/libLLVMCore.so.14git)
ld: fatal: symbol referencing errors

Solaris `ld` requires to directly link with dependant libraries, so this patch explicitly adds
`libLLVMCore`.

Tested on `amd64-pc-solaris2.11` and `x86_64-pc-linux-gnu`.


Repository:
rG LLVM Github Monorepo

https://reviews.llvm.org/D118452

Files:
polly/unittests/DeLICM/CMakeLists.txt


Index: polly/unittests/DeLICM/CMakeLists.txt
===================================================================
--- polly/unittests/DeLICM/CMakeLists.txt
+++ polly/unittests/DeLICM/CMakeLists.txt
@@ -1,3 +1,7 @@
+set(LLVM_LINK_COMPONENTS
+ Core
+ )
+
add_polly_unittest(DeLICMTests
DeLICMTest.cpp
)

[Michael Kruse via Phabricator]

Meinersbur added a comment.

DeLICMTests, like all the unittests indeed requires LLVMCore, but I would have assumed it is resolved as transitive dependency by cmake, or at least `llvm_config` inside `add_polly_unittest`.

On Linux/x86_64, this configuration ist tested by https://lab.llvm.org/buildbot/#/builders/207 which does not show this issue. Are "implicit dependencies" something Solaris-specific?



================
Comment at: polly/unittests/DeLICM/CMakeLists.txt:1
+set(LLVM_LINK_COMPONENTS
+ Core
----------------
Before I accept the patch, could you add a comment saying why this is needed?

Repository:
rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D118452/new/

https://reviews.llvm.org/D118452

[Rainer Orth via Phabricator]

ro added a comment.

In D118452#3279208 <https://reviews.llvm.org/D118452#3279208>, @Meinersbur wrote:

> DeLICMTests, like all the unittests indeed requires LLVMCore, but I would have assumed it is resolved as transitive dependency by cmake, or at least `llvm_config` inside `add_polly_unittest`.
>
> On Linux/x86_64, this configuration ist tested by https://lab.llvm.org/buildbot/#/builders/207 which does not show this issue. Are "implicit dependencies" something Solaris-specific?

This only happens on Solaris with `-DBUILD_SHARED_LIBS=ON`, which is `OFF` by default. I guess this requirement is Solaris-specific, yes. In a way, it makes sense: imagine the direct dependeny (which currently depends on `libLLVMCore`) drops that dependency in the future (not likely in this case, but well possible in general). Then the executable would suddenly fail to run, which cannot happen if you make the dependency explicit.

This instance is the only case of this problem in the whole tree (well, the parts I do build on Solaris).

================
Comment at: polly/unittests/DeLICM/CMakeLists.txt:1
+set(LLVM_LINK_COMPONENTS
+ Core
----------------

Meinersbur wrote:
> Before I accept the patch, could you add a comment saying why this is needed?

Sure, will do.

[Rainer Orth via Phabricator]

ro updated this revision to Diff 403986.
ro added a comment.

Add comment.

Repository:
rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D118452/new/

https://reviews.llvm.org/D118452

Files:
polly/unittests/DeLICM/CMakeLists.txt


Index: polly/unittests/DeLICM/CMakeLists.txt
===================================================================
--- polly/unittests/DeLICM/CMakeLists.txt
+++ polly/unittests/DeLICM/CMakeLists.txt

@@ -1,3 +1,8 @@
+# Solaris ld requires this dependency to be made explicit for shared builds.

[Rainer Orth via Phabricator]

ro marked an inline comment as done.

[Michael Kruse via Phabricator]

Meinersbur accepted this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.

LGTM, thank you.

[Rainer Orth via Phabricator]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG15a3476f3f94: [polly][unittests] Link DeLICMTests with libLLVMCore (authored by ro).

Repository:
rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D118452/new/

https://reviews.llvm.org/D118452