Skip to content

[flang] Simplify LIBTYPE logic #98072

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 12, 2024
Merged

Conversation

Meinersbur
Copy link
Member

@Meinersbur Meinersbur commented Jul 8, 2024

When the add_flang_library was first added, it was apparently copied over from add_clang_library, including its logic to determine the library type. It includes a workaround: If BUILD_SHARED_LIBS is enabled, it should build all libraries as shared, including those that are explicitly marked as STATIC1, because add_clang_library always passes at least one of STATIC/SHARED to llvm_add_library, and llvm_add_library could not distinguish the two cases.

Then, the two implementations diverged. For its runtime libraries, Flang requires some libraries to always be static libraries, so if a library is explicitly marked as STATIC, BUILD_SHARED_LIBS is ignored2.
 
I noticed the two implementations of the same functionality, modified only the add_clang_library, and copied over the result to add_flang_library3, without noticing that they are slightly different. As a result, Flang runtime libraries would be built as shared libraries with -DBUILD_SHARED_LIBS=ON, which may break some build configurations4.

This PR fixes the problem and at the same time simplifies the library type algorithm by just passing SHARED/STATIC verbatim to llvm_add_library. This is effectively what 2 should have done instead adding more code to undo the workaround of 1. Ideally, one would use

llvm_add_library(${name} ${ARG_STATIC} ${ARG_SHARED} [...])

but ARG_STATIC/ARG_SHARED as set by cmake_parse_arguments contain TRUE/FALSE instead of the keywords themselves. I could imagine a utility function akin to pythonize_bool that does this.

This simplification adds two more changes:

  1. Object libraries are not explicitly requested anymore. llvm_add_library itself should determine whether an object library is necessary. As the comment notes, using an object library is not without problems and seem of no use here since it works fine without object library for XCODE/MSVC_IDE.

  2. The property CLANG_STATIC_LIBS was removed. It was FLANG_STATIC_LIBS before to copy&paste error of Avoid object libraries in the VS IDE #93519 3 which not used anywhere. In clang, CLANG_STATIC_LIBS is used for clang-shlib to include all component libraries in a single large library. There is no equivalent flang-shlib.

Footnotes

  1. dbc2a12 2

  2. 3d2e05d 2

  3. Avoid object libraries in the VS IDE #93519 2

  4. https://github.com/llvm/llvm-project/pull/93519#issuecomment-2192359002

@Meinersbur
Copy link
Member Author

To elaborate on the idea of making forwarding of flags easier:

function(bool_argument_as_forwardable_flag prefix flag)
  if ("${${prefix}_${flag}}")
    set("${prefix}_${flag}" "${flag}" PARENT_SCOPE)
  else()
    set("${prefix}_${flag}" "" PARENT_SCOPE)
  endif()
endfunction()

bool_argument_as_forwardable_flag(ARG STATIC)
bool_argument_as_forwardable_flag(ARG SHARED)
llvm_add_library(${name} ${ARG_STATIC} ${ARG_SHARED} [...])

@Meinersbur Meinersbur marked this pull request as ready for review July 11, 2024 15:23
@llvmbot llvmbot added the flang Flang issues not falling into any other category label Jul 11, 2024
Copy link
Contributor

@dpalermo dpalermo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have verified this fixes the issue with the llvm-flang build when the cmake flag is specified: -DFLANG_RUNTIME_F128_MATH_LIB=libquadmath

@Meinersbur Meinersbur merged commit 424ad16 into llvm:main Jul 12, 2024
11 checks passed
aaryanshukla pushed a commit to aaryanshukla/llvm-project that referenced this pull request Jul 14, 2024
When the `add_flang_library` was first added, it was apparently copied
over from `add_clang_library`, including its logic to determine the
library type. It includes a workaround: If `BUILD_SHARED_LIBS` is
enabled, it should build all libraries as shared, including those that
are explicitly marked as `STATIC`[^1], because `add_clang_library`
always passes at least one of `STATIC`/`SHARED` to `llvm_add_library`,
and `llvm_add_library` could not distinguish the two cases.

Then, the two implementations diverged. For its runtime libraries, Flang
requires some libraries to always be static libraries, so if a library
is explicitly marked as `STATIC`, `BUILD_SHARED_LIBS` is ignored[^2].
 
I noticed the two implementations of the same functionality, modified
only the `add_clang_library`, and copied over the result to
`add_flang_library`[^3], without noticing that they are slightly
different. As a result, Flang runtime libraries would be built as shared
libraries with `-DBUILD_SHARED_LIBS=ON`, which may break some build
configurations[^4].

This PR fixes the problem and at the same time simplifies the library
type algorithm by just passing SHARED/STATIC verbatim to
`llvm_add_library`. This is effectively what [^2] should have done
instead adding more code to undo the workaround of [^1]. Ideally, one
would use
```
llvm_add_library(${name} ${ARG_STATIC} ${ARG_SHARED} [...])
```
but `ARG_STATIC`/`ARG_SHARED` as set by `cmake_parse_arguments` contain
`TRUE`/`FALSE` instead of the keywords themselves. I could imagine a
utility function akin to `pythonize_bool` that does this.

This simplification adds two more changes:

1. Object libraries are not explicitly requested anymore.
`llvm_add_library` itself should determine whether an object library is
necessary. As the comment notes, using an object library is not without
problems and seem of no use here since it works fine without object
library when in `XCODE`/`MSVC_IDE` mode.

2. The property `CLANG_STATIC_LIBS` was removed. It was
`FLANG_STATIC_LIBS` before to copy&paste error of llvm#93519 [^3] which not
used anywhere. In clang, `CLANG_STATIC_LIBS` is used for `clang-shlib`
to include all component libraries in a single large library. There is
no equivalent `flang-shlib`.

[^1]: dbc2a12

[^2]: 3d2e05d

[^3]: llvm#93519

[^4]:
llvm#93519 (comment)
Zentrik added a commit to Zentrik/llvm-project that referenced this pull request Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants