Page MenuHomePhabricator

[flang] Enabling flang to build with BUILD_SHARED_LIBS=On
AcceptedPublic

Authored by Renaud-K on Jun 2 2021, 10:53 AM.

Details

Summary

When building with static libs, the linker only needs to find the minimal dependencies required to arrive to an executable.
When building with shared libs, the linker requires that all the symbols in the shared libs be resolved which exposes more dependencies.

Making the dependency with FortranEvaluate explicit so we can build with BUILD_SHARED_LIBS=On

Diff Detail

Unit TestsFailed

TimeTest
20 msx64 debian > MLIR.mlir-lsp-server::diagnostics.test
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/mlir-lsp-server -lit-test < /var/lib/buildkite-agent/builds/llvm-project/mlir/test/mlir-lsp-server/diagnostics.test | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck -strict-whitespace /var/lib/buildkite-agent/builds/llvm-project/mlir/test/mlir-lsp-server/diagnostics.test
40 msx64 windows > MLIR.mlir-lsp-server::diagnostics.test
Script: -- : 'RUN: at line 1'; c:\ws\w6\llvm-project\premerge-checks\build\bin\mlir-lsp-server.exe -lit-test < C:\ws\w6\llvm-project\premerge-checks\mlir\test\mlir-lsp-server\diagnostics.test | c:\ws\w6\llvm-project\premerge-checks\build\bin\filecheck.exe -strict-whitespace C:\ws\w6\llvm-project\premerge-checks\mlir\test\mlir-lsp-server\diagnostics.test

Event Timeline

Renaud-K created this revision.Jun 2 2021, 10:53 AM
Renaud-K requested review of this revision.Jun 2 2021, 10:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2021, 10:53 AM

Hi @Renaud-K , thank you for submitting this!

Interestingly, flang-aarch64-ubuntu-sharedlibs is fine. This Buildbot worker was set-up specifically to test BUILD_SHARED_LIBS. Could you double-check whether this change is really needed? I will also test tomorrow.

Renaud-K added a comment.EditedJun 2 2021, 4:55 PM

The change does NOT appear to be necessary for main of llvm-project, however I could reproduce it with fir-dev branch of f18-llvm-project. I was told that changes to this file should go through phabricator and percolate to fir-dev. I am not too sure what to do now.

schweitz accepted this revision.Jun 2 2021, 5:01 PM
This revision is now accepted and ready to land.Jun 2 2021, 5:01 PM

The change does NOT appear to be necessary for main of llvm-project, however I could reproduce it with fir-dev branch of f18-llvm-project. I was told that changes to this file should go through phabricator and percolate to fir-dev.

I don't understand that. If there is a build issue on fir-dev but not on main then the bugfix only needs to go on fir-dev and not main. When the extra code on fir-dev that causes the bug comes to main then the bugfix can come with it at that point. main has buildbots for shared libs that ensure we notice it if that happens.

Renaud-K added a comment.EditedJun 3 2021, 1:12 PM

I have been looking some more into how the flang-new driver is being built.
I see that flangFrontend and flangFrontendTool libraries have been introduced and include dependencies with the Fortran libraries. It makes things look elegant. When we link, we just have to say flangFrontend and we get all the Fortran libraries. However when building with shared libs , you can see that the flangFrontEnd libraries are very small, they no longer include any of the code they depend on. So the attempt to have an umbrella library does not work because the linker does not link with the dependent libraries. In a shared lib flow, you have to make them explicit.

If we had some very tight "C like" interfaces, you could guarantee that the driver only makes calls into the objects of the flangFrontend libraries and this could work. But with all the rich header files included which have inlined templated code you have no control over what gets called and it is only a matter of time until this breaks. Here is the change, precisely single header file change, from the fir-dev branch that is causing the link to fail. It is a very small, very reasonable change. https://github.com/flang-compiler/f18-llvm-project/blame/fir-dev/flang/include/flang/Evaluate/expression.h#L97

I guess we could make it so the call is not inlined. But my feeling is that the umbrella library approach is brittle and that this issue will come up again.

I see that flangFrontend and flangFrontendTool libraries have been introduced and include dependencies with the Fortran libraries.

No. These libraries implement the frontend driver. They depend on the frontend libraries, because they "drive" it.

However when building with shared libs , you can see that the flangFrontEnd libraries are very small, they no longer include any of the code they depend on.

This is simply the difference between static and shared libraries, right? There is nothing special about flangFrontend here.

So the attempt to have an umbrella library does not work because the linker does not link with the dependent libraries. In a shared lib flow, you have to make them explicit.

In both cases (static and shared) all dependencies should be expressed explicitly. It just so happens that in the case of static libraries, one can miss a dependency and things will build just fine anyway. That's because linking is delayed until the final binary is build (e.g. flang-new): as long as the missing dependency is also present in the final binary, everything is fine. So the missing dependency could be coming from yet another library.

I guess we could make it so the call is not inlined. But my feeling is that the umbrella library approach is brittle and that this issue will come up again.

This did indeed come up in the past:

In order to defend Flang from such failures in the future, we've set-up a Buildbot worker that builds with BUILD_SHARED_LIBS set to On.

As you admitted, there is no issue with llvm-project/flang and this change is not required.

Great, it sounds like you've resolved your original issue!