This is an archive of the discontinued LLVM Phabricator instance.

[build] Fix stand-alone builds of clang.
ClosedPublic

Authored by fpetrogalli on Jan 12 2023, 1:59 AM.

Details

Summary
The header file `llvm/include/llvm/Targetparser/RISCVTargetParser.h`
relies on the auto-generated *.inc file associated to the tablegen
target `RISCVTargetParserTableGen`.

Both clangBasic and clangDriver include `RISCVTargetParser.h`,
therefore we need to make sure that the *.inc file is avaiable to
avoid compilation errors like the following:

    FAILED: tools/clang/lib/Basic/CMakeFiles/obj.clangBasic.dir/Targets/RISCV.cpp.o
    /usr/bin/c++  [bunch of non interesting stuff]  -c <path-to>/llvm-project/clang/lib/Basic/Targets/RISCV.cpp
    In file included from <path-to>/llvm-project/clang/lib/Basic/Targets/RISCV.cpp:19:
    <path-to>/llvm-project/llvm/include/llvm/TargetParser/RISCVTargetParser.h:29:10: fatal error: llvm/TargetParser/RISCVTargetParserDef.inc: No such file or directory
      29 | #include "llvm/TargetParser/RISCVTargetParserDef.inc"
         |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The stand-alone build of `clang` has been tested with the following
script (see [*] for further information):

```
build_llvm=`pwd`/build-llvm
build_clang=`pwd`/build-clang
installprefix=`pwd`/install
llvm=`pwd`/llvm-project
mkdir -p $build_llvm
mkdir -p $installprefix

cmake -G Ninja -S $llvm/llvm -B $build_llvm \
      -DLLVM_INSTALL_UTILS=ON \
      -DCMAKE_INSTALL_PREFIX=$installprefix \
      -DCMAKE_BUILD_TYPE=Release

ninja -C $build_llvm install

cmake -G Ninja -S $llvm/clang -B $build_clang \
      -DLLVM_EXTERNAL_LIT=$build_llvm/utils/lit \
      -DLLVM_ROOT=$installprefix
```

[*] https://llvm.org/docs/GettingStarted.html#stand-alone-builds

Differential Revision: https://reviews.llvm.org/D141581

Diff Detail

Event Timeline

fpetrogalli created this revision.Jan 12 2023, 1:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2023, 1:59 AM
fpetrogalli requested review of this revision.Jan 12 2023, 1:59 AM
fpetrogalli added a subscriber: craig.topper.

@jrtc27, @thakis, @craig.topper - is this a better solution to the issue raised in https://reviews.llvm.org/D137517?

FWIW, the change in this patch solves the issue with standalone build of clang reported in https://reviews.llvm.org/rGac1ffd3caca12c254e0b8c847aa8ce8e51b6cfbf. (FYI, @mgorny )

@jrtc27, @thakis, @craig.topper - gentle ping, it would be great if I could unlock @mgorny with this patch for the issue they are seeing at https://reviews.llvm.org/rGac1ffd3caca12c254e0b8c847aa8ce8e51b6cfbf

Francesco

Why is it not sufficient to link to RISCVTargetParserTableGen, but is sufficient to link to LLVMTargetParser?
Does RISCVTargetParserTableGen itself not link to LLVMTargetParser?

I though Clang Basic is a leaf library and must not depend on anything.

Why is it not sufficient to link to RISCVTargetParserTableGen, but is sufficient to link to LLVMTargetParser?

This is because the sources of clangBasic and clangDriver might be compiled before LLVMTargetParser is ready. In this case, compilation would fail because both libraries include the header file llvm/TargetParser/RISCVTargetParser.h, which needs the inc file generated by RISCVTargetParserTableGen.

// quoting code from llvm/TargetParser/RISCVTargetParser.h
enum CPUKind : unsigned {
#define PROC(ENUM, NAME, FEATURES, DEFAULT_MARCH) CK_##ENUM,
#define TUNE_PROC(ENUM, NAME) CK_##ENUM,
#include "llvm/TargetParser/RISCVTargetParserDef.inc"
};

Does RISCVTargetParserTableGen itself not link to LLVMTargetParser?

Nope, there is no "linking" between these two components. It is just that LLVMTargetParser requires RISCVTargetParserTableGen. Therefore, if we say that clangDriver and clangBasic depend on LLVMTargetParser we make sure that the inclusion of the tablegen-generated file resolves correctly.

I though Clang Basic is a leaf library and must not depend on anything.

I wasn't aware of this requirement. As of https://reviews.llvm.org/D137517 it depends on some auto-generated target information stored in LLVMTargetParser and llvm/lib/Target/Target/RISCV/RISCV.td. At the end, it is not even depending on the LLVMTargetParser library, it is just one header file in the public interface of the TargetParser that is needed (see explanation in https://reviews.llvm.org/D141581#4056503)

This is because the sources of clangBasic and clangDriver might be compiled before LLVMTargetParser is ready.

...

Therefore, if we say that clangDriver and clangBasic depend on LLVMTargetParser we make sure that the inclusion of the tablegen-generated file resolves correctly.

Sorry, I don't follow. If I read correctly, you're saying that clang libraries might begin to compile before their DEPENDS dependency is built (implying that DEPENDS clause only guarantees that the dependency is ready at link stage). If it is true, the proposed patch changes nothing -- the sources might still start to compile before cmake decides to generate inc file, because it is only needed at link stage.
Am I missing something?

This is because the sources of clangBasic and clangDriver might be compiled before LLVMTargetParser is ready.

...

Therefore, if we say that clangDriver and clangBasic depend on LLVMTargetParser we make sure that the inclusion of the tablegen-generated file resolves correctly.

Sorry, I don't follow. If I read correctly, you're saying that clang libraries might begin to compile before their DEPENDS dependency is built (implying that DEPENDS clause only guarantees that the dependency is ready at link stage). If it is true, the proposed patch changes nothing -- the sources might still start to compile before cmake decides to generate inc file, because it is only needed at link stage.
Am I missing something?

Ops, yeah, I think I am wrong. In fact , when RISCVTargetParserTableGen is in the DEPENDS, it should be built before the clangBasic start to compile. This means that I actually do not know why this change fixes the issue reported

@mgorny - can you try this patch on your workflow? Maybe the issue disappeared in my local machine just because for some reason it changed the order of compilation?

fpetrogalli added a comment.EditedJan 16 2023, 8:57 AM

@barannikov88 - I am stuck with an incomplete explanation:

The issue raised by @mgorny is about stand-alone builds. In this build configuration, clang is built separately from LLVM, which is first build and installed in a folder.

When clang is build as a stand-alone project, clang does not have access to the cmake configuration of LLVM. Therefore it knows nothing about the tablegen target RISCVTargetParserTableGen.

The's why we see the following error when building stand-alone clang on main:

CMake Error at /Users/fpetrogalli/projects/cpu-defs/install/lib/cmake/llvm/AddLLVM.cmake:536 (add_dependencies):
  The dependency target "RISCVTargetParserTableGen" of target
  "obj.clangBasic" does not exist.
Call Stack (most recent call first):
  cmake/modules/AddClang.cmake:106 (llvm_add_library)
  lib/Basic/CMakeLists.txt:40 (add_clang_library)

I do not know why LLVMTargetParser (which is also specified in the cmake configuration of llvm) is instead recognised as a valid dependencies of clang on some LLVM library. Maybe because cmake looks in the prefix folder where LLVM was installed and finds the object file of the library?

FWIW, I was able to reproduce the issue reported by @mgorny via the following script when run on main:

#!/bin/sh

build_llvm=`pwd`/build-llvm
build_clang=`pwd`/build-clang
installprefix=`pwd`/install
llvm=`pwd`/llvm-project
mkdir -p $build_llvm
mkdir -p $installprefix

cmake -G Ninja -S $llvm/llvm -B $build_llvm \
      -DLLVM_INSTALL_UTILS=ON \
      -DCMAKE_INSTALL_PREFIX=$installprefix \
      -DCMAKE_BUILD_TYPE=Release


ninja -C $build_llvm install

cmake -G Ninja -S $llvm/clang -B $build_clang \
      -DLLVM_EXTERNAL_LIT=$build_llvm/utils/llvm-lit \
      -DLLVM_ROOT=$installprefix

Given it was an issue that cmake itself was reporting (and not a build time failure), I thought that its disappearance after applying this patch was a signal thatI was doing the right thing.

@barannikov88 - I am stuck with an incomplete explanation:

Ah, I see. The key point is that standalone build that depends on installed LLVM.
RISCVTargetParser is a custom target and it is not being installed, while LLVMTargetParser is a "real" target and gets installed. This is probably why changing the dependency fixes the issue.
The change looks good to me then.

clangBasic and clangDriver already have a dependency on TargetParser (see LLVM_LINK_COMPONENTS at the beginning of corresponding files). Is that not enough?
Will it build if you just remove the additional dependency?

clangBasic and clangDriver already have a dependency on TargetParser (see LLVM_LINK_COMPONENTS at the beginning of corresponding files). Is that not enough?
Will it build if you just remove the additional dependency?

No - if we just specify the dependency in LLVM_LINK_COMPONENTS, there are build failures, as explained in the commit message of https://reviews.llvm.org/rGac1ffd3caca12c254e0b8c847aa8ce8e51b6cfbf.

Hi -has anybody any more concern on this change? I'd like to submit it as soon as possible to unlock @mgorny .

Francesco

tstellar added a subscriber: tstellar.EditedJan 20 2023, 7:45 AM

clangBasic and clangDriver already have a dependency on TargetParser (see LLVM_LINK_COMPONENTS at the beginning of corresponding files). Is that not enough?
Will it build if you just remove the additional dependency?

No - if we just specify the dependency in LLVM_LINK_COMPONENTS, there are build failures, as explained in the commit message of https://reviews.llvm.org/rGac1ffd3caca12c254e0b8c847aa8ce8e51b6cfbf.

It's still not clear to me why LLVM_LINK_COMPONENTS does not work. Is LLVMTargetParser a library?

So, what's the actual dependency here? Do libBasic and libDriver just need the header to be generated or does it actually need to link to the library?

[...]
It's still not clear to me why LLVM_LINK_COMPONENTS does not work. Is LLVMTargetParser a library?

Yes, LLVMTargetParser is a library built located in llvm/lib/TargetParser. LLVM_LINK_COMPONENTS does not work if clangBasic or clangDriver start building before LLVMTargetParser, because the header file llvm/TargetParser/RISCVTargetParserDefs.inc has not been generated yet. This didn't happen on my local build, but on some bot I had the following error reported even if TargetParser was added to LLVM_LINK_COMPONENTS.

<path-to>/llvm-project/llvm/include/llvm/TargetParser/RISCVTargetParser.h:29:10: fatal error: llvm/TargetParser/RISCVTargetParserDef.inc: No such file or directory

29 | #include "llvm/TargetParser/RISCVTargetParserDef.inc"
   |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

By adding LLVMTargetParser to the DEPENDS of clangBasic and clangDriver it seems that the order of building LLVMTargetParser before any of the dependents is enforced.

To put in other words, it seems that just specifying` LINK_COMPONENTS = A` for a library B allows A and B to be built in parallel.

So, what's the actual dependency here? Do libBasic and libDriver just need the header to be generated or does it actually need to link to the library?

Just the header.

I don't think this is the correct way to specify dependencies if it's just an issue of the header being included before a generated file it needs has been generated. Are there other places in the code where a generated header file is included by another header?

I don't think this is the correct way to specify dependencies if it's just an issue of the header being included before a generated file it needs has been generated. Are there other places in the code where a generated header file is included by another header?

% grep -r "RISCVTargetParser.h" *
include/llvm/module.modulemap:    header "TargetParser/RISCVTargetParser.h"
lib/Target/RISCV/RISCVISelLowering.h:#include "llvm/TargetParser/RISCVTargetParser.h"
lib/TargetParser/RISCVTargetParser.cpp:#include "llvm/TargetParser/RISCVTargetParser.h"

RISCVTargetParser.h is the one that references the generated file via :

enum CPUKind : unsigned {
#define PROC(ENUM, NAME, FEATURES, DEFAULT_MARCH) CK_##ENUM,
#define TUNE_PROC(ENUM, NAME) CK_##ENUM,
#include "llvm/TargetParser/RISCVTargetParserDef.inc"
};

I spent some time looking at this. Rather than changing the dependency from RISCVTargetParserTableGen to LLVMTargetParser, I think the correct fix is to handle RISCVTargetParserTableGen in llvm/cmake/modules/LLVMConfig.cmake.in. alongside the intrinsics_gen, omp_gen, and acc_gen targets.

kwk added a subscriber: kwk.Jan 20 2023, 11:47 AM

@tstellar as you suspected, I had the following problem when building clang in standalone mode and your patch solved it for me. Thank you.

-- Configuring done
CMake Error at /usr/lib64/cmake/llvm/AddLLVM.cmake:536 (add_dependencies):
  The dependency target "RISCVTargetParserTableGen" of target
  "obj.clangBasic" does not exist.
Call Stack (most recent call first):
  cmake/modules/AddClang.cmake:106 (llvm_add_library)
  lib/Basic/CMakeLists.txt:40 (add_clang_library)


CMake Error at /usr/lib64/cmake/llvm/AddLLVM.cmake:719 (add_dependencies):
  The dependency target "RISCVTargetParserTableGen" of target "clangBasic"
  does not exist.
Call Stack (most recent call first):
  cmake/modules/AddClang.cmake:106 (llvm_add_library)
  lib/Basic/CMakeLists.txt:40 (add_clang_library)


CMake Error at /usr/lib64/cmake/llvm/AddLLVM.cmake:536 (add_dependencies):
  The dependency target "RISCVTargetParserTableGen" of target
  "obj.clangDriver" does not exist.
Call Stack (most recent call first):
  cmake/modules/AddClang.cmake:106 (llvm_add_library)
  lib/Driver/CMakeLists.txt:17 (add_clang_library)


CMake Error at /usr/lib64/cmake/llvm/AddLLVM.cmake:719 (add_dependencies):
  The dependency target "RISCVTargetParserTableGen" of target "clangDriver"
  does not exist.
Call Stack (most recent call first):
  cmake/modules/AddClang.cmake:106 (llvm_add_library)
  lib/Driver/CMakeLists.txt:17 (add_clang_library)

This is currently holding back further testing on our end which is concerning me a bit, especially as we approach the branch point. Could you revert this please if a fix isn't imminent? Thank you!

kwk added a comment.Jan 23 2023, 2:05 AM

This is currently holding back further testing on our end which is concerning me a bit, especially as we approach the branch point. Could you revert this please if a fix isn't imminent? Thank you!

@thesamesam what do you mean with "revert"? Just by looking at this differential I cannot see that this patch has landed. Has it?

This is currently holding back further testing on our end which is concerning me a bit, especially as we approach the branch point. Could you revert this please if a fix isn't imminent? Thank you!

@thesamesam what do you mean with "revert"? Just by looking at this differential I cannot see that this patch has landed. Has it?

He was referring to reverting ac1ffd3caca12c254e0b8c847aa8ce8e51b6cfbf, i.e. the change that introduced the problem. However, FWICS reverting it is non-trivial and will probably cause quite a mess.

So, well, we're stuck for two weeks now being unable to test new LLVM versions, the branching point is approaching fast and it's quite likely we'll have to put a lot of effort afterwards to fix everything and request backporting our fixes to 16.x.

@mgorny, I have updated the patch according to the suggestions from @tstellar in https://reviews.llvm.org/D141581#4069857.

I have used the script below to test the stand-alone build of clang. Indeed, the error related to the cmake configuration for RISCVTargetParserDefs has disappeared. However, there is an (unreltaed) error that does not allow me to test the full build:

CMake Error at /Users/fpetrogalli/projects/cpu-defs/install/lib/cmake/llvm/LLVMExports.cmake:1222 (set_target_properties):
  The link interface of target "LLVMLineEditor" contains:

    LibEdit::LibEdit

  but the target was not found.  Possible reasons include:

    * There is a typo in the target name.
    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.

Call Stack (most recent call first):
  /Users/fpetrogalli/projects/cpu-defs/install/lib/cmake/llvm/LLVMConfig.cmake:329 (include)
  CMakeLists.txt:38 (find_package)

Script used for testing:

% cat standalone.sh
#!/bin/sh

build_llvm=`pwd`/build-llvm
build_clang=`pwd`/build-clang
installprefix=`pwd`/install
llvm=`pwd`/llvm-project
mkdir -p $build_llvm
mkdir -p $installprefix

cmake -G Ninja -S $llvm/llvm -B $build_llvm \
      -DLLVM_INSTALL_UTILS=ON \
      -DCMAKE_INSTALL_PREFIX=$installprefix \
      -DCMAKE_BUILD_TYPE=Release

ninja -C $build_llvm install

cmake -G Ninja -S $llvm/clang -B $build_clang \
      -DLLVM_EXTERNAL_LIT=$build_llvm/utils/lit \
      -DLLVM_ROOT=$installprefix
Herald added a project: Restricted Project. · View Herald Transcript
fpetrogalli edited the summary of this revision. (Show Details)Jan 23 2023, 5:33 AM
fpetrogalli retitled this revision from [clang] Make clangBasic and clangDriver depend on LLVMTargetParser. to [build] Fix stand-alone builds of clang..
tstellar added inline comments.Jan 23 2023, 10:20 AM
clang/lib/Basic/CMakeLists.txt
113

Doesn't this break the monorepo (non-standalone) build? If the monorepo build still works with this, then you don't need the changes to llvm/cmake/modules/LLVMConfig.cmake.in

tstellar accepted this revision.Jan 23 2023, 10:39 AM

I tested this patch and it works, so I'll give it an LGTM. However, I think the LLVMConfig.cmake.in changes are now unnecessary since the RISCVTargetParserTableGen has been dropped from all clang targets.

This revision is now accepted and ready to land.Jan 23 2023, 10:39 AM
fpetrogalli added inline comments.Jan 23 2023, 10:42 AM
clang/lib/Basic/CMakeLists.txt
113

This configuration built without problems on my machine.

cmake ../llvm-project/llvm -DLLVM_ENABLE_PROJECTS=clang -DCMAKE_BUILD_TYPE=Release -G Ninja

I tested this patch and it works, so I'll give it an LGTM. However, I think the LLVMConfig.cmake.in changes are now unnecessary since the RISCVTargetParserTableGen has been dropped from all clang targets.

I am tempted to submit it as it is and create a new patch that removes the bits in llvm/cmake/modules/LLVMConfig.cmake.in. As far s I can tell, if we specify the library dependency only in LLVM_LINK_COMPONENTS, there are issue when building non stand-alone clang, as building the clang components might start before the actual header file is generated. I'll submit this tomorrow (if I don't get negative feedback), and then create a new patch that removes the bits in llvm/cmake/modules/LLVMConfig.cmake.in.

fpetrogalli marked an inline comment as done.Jan 23 2023, 12:59 PM

@fpetrogalli Please go ahead and push this since it is blocking others, we can fix up any of the other issues later.

This revision was landed with ongoing or failed builds.Jan 23 2023, 1:12 PM
This revision was automatically updated to reflect the committed changes.

Ah - there you go. Without a proper chained dependency of LLVMTargetParser -> clangBasic we end up with failures if clangBasic is compiled before LLVMTargetParser is (even on non stand-alone builds). https://lab.llvm.org/buildbot/#/builders/193/builds/25362 :

ccache /usr/bin/c++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/home/omp-vega20-0/bbot/openmp-offload-amdgpu-runtime/llvm.build/tools/clang/lib/Basic -I/home/omp-vega20-0/bbot/openmp-offload-amdgpu-runtime/llvm.src/clang/lib/Basic -I/home/omp-vega20-0/bbot/openmp-offload-amdgpu-runtime/llvm.src/clang/include -I/home/omp-vega20-0/bbot/openmp-offload-amdgpu-runtime/llvm.build/tools/clang/include -I/home/omp-vega20-0/bbot/openmp-offload-amdgpu-runtime/llvm.build/include -I/home/omp-vega20-0/bbot/openmp-offload-amdgpu-runtime/llvm.src/llvm/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wno-misleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -fno-strict-aliasing -O3 -DNDEBUG  -fno-exceptions -fno-rtti -UNDEBUG -std=c++17 -MD -MT tools/clang/lib/Basic/CMakeFiles/obj.clangBasic.dir/Targets/RISCV.cpp.o -MF tools/clang/lib/Basic/CMakeFiles/obj.clangBasic.dir/Targets/RISCV.cpp.o.d -o tools/clang/lib/Basic/CMakeFiles/obj.clangBasic.dir/Targets/RISCV.cpp.o -c /home/omp-vega20-0/bbot/openmp-offload-amdgpu-runtime/llvm.src/clang/lib/Basic/Targets/RISCV.cpp
In file included from /home/omp-vega20-0/bbot/openmp-offload-amdgpu-runtime/llvm.src/clang/lib/Basic/Targets/RISCV.cpp:19:
/home/omp-vega20-0/bbot/openmp-offload-amdgpu-runtime/llvm.src/llvm/include/llvm/TargetParser/RISCVTargetParser.h:29:10: fatal error: llvm/TargetParser/RISCVTargetParserDef.inc: No such file or directory
   29 | #include "llvm/TargetParser/RISCVTargetParserDef.inc"
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
2.225 [3029/31/825] Building CXX object tools/clang/lib/Basic/CMakeFiles/obj.clangBasic.dir/Version.cpp.o
2.524 [3029/30/826] Building RISCVTargetParserDef.inc...

I'll revert D141581, as it has chance to fail more bots than the original code.

FWIW, I am out of options. We need to make sure that LLVMTargetParser (or just RISCVTargetParserTablegen) are built - or better, the header file llvm/include/llvm/TargetParser/RISCVTargetParser.h is fully available with its .inc inclusion _before_ clangBasic and clangDriver are compiled.

This patch shows that just specifying the dependency of TargetParser in LLVM_LINK_COMPONENTS is not enough: we need to specify in the DEPENDS on clangDriver and clangBasic the tablegen target or LLVMTargetParser itself.

What I do not understand is why adding DEPENDS LLVMTargetParser (which seems to fix both stand-alone and not) to the clang libraries is not acceptable.

@fpetrogalli The solution is to add RISCVTargetParserTableGen to the Depnds list of clang/lib/Driver and clang/lib/Basic.