[PATCH] D89518: [windows-itanium] Add Windows Itanium How-To Guide

Wed Nov 11 05:53:14 PST 2020

bd1976llvm marked 7 inline comments as done.
bd1976llvm added inline comments.

================
Comment at: llvm/docs/HowToBuildWindowsItaniumPrograms.rst:39
+
+link.exe (the MS linker) is unsuitable as it rejects the COMDATs produced by
+LLVM. It also doesn't support auto-importing which is currently required.
----------------
mstorsjo wrote:
> Did you look into why it produces such comdats? Because for msvc triple targets, it should work with link.exe.
I think I conflated several issues here, as I was trying to get the build set up, and got myself confused. Originally, I think I was somehow generating the "leaderless comdats" that you mentioned in another thread. I had thought that the leaderless comdats were related to multiply defined symbols being present. Happily, I no longer see the leaderless comdats and the remaining multiply defined symbol problems have now been solved and were unrelated. I can now link with MS's link.exe and I have adjusted the doc accordingly.

There is a remaining comdat issue that I see when building libc++ when not using the internal assembler which occurs if I try to use the ' -save-temps=obj' option for debugging. Errors are emitted such as:

    #                It seems that source -> assembly -> obj may report comdat
    #                errors (when assembling) that are not reported doing source-> obj.
    #                Example:
    #                  src/CMakeFiles/cxx_shared.dir\locale.s:107958:2: error: section '.xdata$' is already linkonce
    #                  .linkonce       discard

I doubt this is related to Windows Itanium and I haven't looked into it more as it isn't currently blocking me.

================
Comment at: llvm/docs/HowToBuildWindowsItaniumPrograms.rst:53
+addresses are patched into the IAT). Therefore, the compiler must emit some code,
+that runs after IAT patching but before anything that might use the vtable pointers,
+and sets the vtable pointer to the address from the IAT. For the special case of
----------------
mstorsjo wrote:
> Actually - the code for patching it isn't emitted by either compiler or linker, it's done by the `_pei386_runtime_relocator` function in the mingw runtime. The linker just generates a list of fixups that the runtime later will need to process. If the runtime that processes this list isn't linked in, it won't work. Unfortunately, this can happen silently...
> 
> Have you checked this aspect, that it actually works as intended?
> 
> See https://github.com/mstorsjo/llvm-project/commit/lld-pseudo-relocs for a patch (not recently rebased, unfortunately) to lld that makes it force a reference to this function, if runtime fixups actually are needed. That would make it clear if it's needed, but missing.
> 
> Finally, the `_pei386_runtime_relocator` function also needs to be called. Currently in the mingw-w64 runtime, it's always called unconditionally by other startup code, but with a change like https://github.com/mstorsjo/mingw-w64/commit/pseudo-reloc-ctor, that object file would have a ctor that forces it to be called autonomously, if the object file is linked in. (Unfortunately, that change doesn't work with ld.bfd as it is right now, so it isn't upstreamed.)
It does not work as intended. Thanks for providing details here. I have adjusted the doc to add a more detailed explanation. As I understand it one of the goal of Windows Itanium is to use the Itanium C++ ABI on windows without relying on extra dependencies such as  a custom runtime. Therefore, this must remain as a limitation for now.  I spoke to @compnerd briefly about this problem and he though that the solution would be in the compiler emitting code that is called by the MS runtime at start-up and initialises these fields correct. I haven't yet investigated whether this is feasible or not.

================
Comment at: llvm/docs/HowToBuildWindowsItaniumPrograms.rst:57
+there is no declaration available to the compiler so this can't be done. To allow
+programs to link we currently rely on the -lldmingw switch in LLD to auto-import
+references to __cxxabiv1::__class_type_info pointers (see: https://reviews.llvm.org/D43184
----------------
mstorsjo wrote:
> Fwiw, since release 11, you can enable autoimporting alone without enabling the rest of the mingw specific quirks, with the `-autoimport` flag. Also see D89006 for a recent related fixup.
Thanks. This is a nice improvement!

================
Comment at: llvm/docs/HowToBuildWindowsItaniumPrograms.rst:116
+
+The CMake files will need to be edited to prevent them adding GNU specific libraries to the link line.
+
----------------
mstorsjo wrote:
> For my builds, as long as I'm using `LIBUNWIND_USE_COMPILER_RT`, it links clang-rt.builtins instead of libgcc, but I guess there's a need for more options there.
I'll look into upstreaming something for this. Currently, not adding these libraries is an easy edit to make tothe CMake files so I'm not worried about it.

================
Comment at: llvm/docs/HowToBuildWindowsItaniumPrograms.rst:127
+into the libc++ DLL. This necessitates editing the CMake file
+to ensure that the visibility macros (which expand to dllexport/import)
+are expanded as they will be needed when creating the final libc++
----------------
mstorsjo wrote:
> I hadn't noticed this wording here before, but this editing of cmake files to tweak visibility macros pretty much amounts to https://reviews.llvm.org/D90021 I guess? Hopefully that one (in whichever form I tweak it to) works for your case as well, to avoid any need for manual modifications.
Great - thanks for putting that up!

================
Comment at: llvm/docs/HowToBuildWindowsItaniumPrograms.rst:176
+
+* ``-DCMAKE_C_FLAGS="-Xlinker /force:multiple"``
+
----------------
mstorsjo wrote:
> Why is that needed? If it's the same thing I've run into at some point, you can add `-DLIBCXXABI_ENABLE_NEW_DELETE_DEFINITIONS=OFF` to avoid the redundant symbols.
Thanks - it's not needed. There was a simple fix to the build that I had missed.

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

https://reviews.llvm.org/D89518