[libcxx-commits] [PATCH] D154869: [Flang] [FlangRT] Implement FlangRT library as solution to Flang's runtime LLVM integration

[Andrzej Warzynski via Phabricator via libcxx-commits]

awarzynski added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:934-936
+  // Default to the <driver-path>/../lib and
+  // <driver-path>/../runtimes/runtimes-bins/lib directories. This works fine
+  // on the platforms that we have tested so far. We will probably have to
----------------
pscoro wrote:
> awarzynski wrote:
> > Am I correct thinking that:
> > * "<driver-path>/../lib" is for `Fortran_main.a`, and
> > * "<driver-path>/../runtimes/runtimes-bins/lib " is for `libflang-rt`?
> > 
> > Could you document this? Thanks!
> Yep thats right, documentation added
> Yep thats right, documentation added

Could you document _where_ these libraries are located? Also, could you add some relevant comments here? For example:
```
  // Add search path for XYZ
  llvm::sys::path::append(BuildLibPath, "lib");
```


================
Comment at: flang/docs/Flang-rt.md:15-16
+```
+The flang driver requires two libraries for linking into any user executable,
+`Fortran_main.a` and Flang-rt. 
+## Motivation
----------------
This statement is not quite true - the driver can do a lot without requiring these libraries. I suspect that you wanted o say that generating executables requires these libraries.

[nit] flang dirver --> Flang driver


================
Comment at: flang/docs/Flang-rt.md:17
+`Fortran_main.a` and Flang-rt. 
+## Motivation
+Before Flang-rt, the driver would need to link Fortran_main, FortranRuntime,
----------------
Motivation for ...? It's not clear what the following section aims to achieve.

I think that it would be good to take a step beck and decide who this documented is intended for. If it's the end-user then, imho, this section doesn't belong here. In documentation like this I'd focus on "what" and "how" as opposed to "why". 

Even if this document is intended for developers, I wouldn't focus too much on the original motivations for this change and instead explain the design and its benefits:
* possibility to build the runtime independently of Flang (what's the benefit of that?)
* CMake set-up that's consistent with other sub-projects in LLVM,
* a convenient wrapper for multiple (i.e. 2) Flang runtime libraries,
* automated logic to build shared and static version of the runtime libs (to enable supporting e.g. `-static`?).

I hope that this makes sense.


================
Comment at: flang/docs/Flang-rt.md:30-31
+## Fortran_main
+Fortran_main is left out of Flang-rt because it is required to be linked
+statically. The link type of Flang-rt can be configured with a CMake option.
+
----------------
Could you document "why"?


================
Comment at: flang/docs/Flang-rt.md:35
+In order to decouple the common dependency between compiler and runtime,
+FortranDecimal's sources get built a second time into a library target named
+FortranDecimalRT. FortranDecimal is built for the compiler and FortranDecimalRT
----------------
Perhaps turn this into a link to the CMake definition?


================
Comment at: flang/docs/Flang-rt.md:61-63
+The two build paths mentioned above get implicitly added as library paths at the
+invocation of the driver. If Flang-rt is a shared library, you must add its path
+to the environment variable `LD_LIBRARY_PATH`.
----------------
1. Please add an example.
2. "you must add its path to the environment variable `LD_LIBRARY_PATH`" <-- this is just one way to provide a search path for the dynamic linker. There are other ways too. It would be good to clarify that a) one has to make the dynamic linker aware _where_ to look and that b) `LD_LIBRARY_PATH` is one such mechanism that can be used. But I would be careful not to "endorse" it - mishandling `LD_LIBRARY_PATH` can lead to tricky surprises.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154869