Page MenuHomePhabricator

[MinGW][llvm-config] Use unversioned library name
AbandonedPublic

Authored by mati865 on Fri, Sep 11, 9:16 AM.

Details

Reviewers
hans
Group Reviewers
Restricted Project
Summary

When building LLVM for MinGW bare LLVM.dll or libLLVM.dll with https://reviews.llvm.org/D87517 is created

Diff Detail

Unit TestsFailed

TimeTest
470 mslinux > Clang.CodeGenObjC::arc-cond-stmt.m
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/12.0.0/include -nostdsysteminc -triple x86_64-apple-darwin10 -emit-llvm -fobjc-arc -o - /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGenObjC/arc-cond-stmt.m
460 mslinux > Clang.CodeGenObjC::arc-weak-property.m
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/12.0.0/include -nostdsysteminc -triple x86_64-apple-darwin10 -emit-llvm -fobjc-runtime-has-weak -fblocks -fobjc-arc -o - /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGenObjC/arc-weak-property.m | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGenObjC/arc-weak-property.m
510 mslinux > Clang.CodeGenObjC::arc.m
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/12.0.0/include -nostdsysteminc -triple x86_64-apple-darwin10 -Wno-objc-root-class -Wno-incompatible-pointer-types -Wno-arc-unsafe-retained-assign -emit-llvm -fblocks -fobjc-arc -fobjc-runtime-has-weak -O2 -disable-llvm-passes -o - /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGenObjC/arc.m | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGenObjC/arc.m
490 mslinux > Clang.CodeGenObjC::atomic-aggregate-property.m
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/12.0.0/include -nostdsysteminc -triple x86_64-apple-darwin10 -fobjc-gc -emit-llvm -o - /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGenObjC/atomic-aggregate-property.m | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck -check-prefix CHECK-LP64 /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGenObjC/atomic-aggregate-property.m
450 mslinux > Clang.CodeGenObjC::debug-info-id-with-protocol.m
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/12.0.0/include -nostdsysteminc -triple x86_64-apple-darwin10 -emit-llvm -debug-info-kind=limited /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGenObjC/debug-info-id-with-protocol.m -o - | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGenObjC/debug-info-id-with-protocol.m
View Full Test Results (18 Failed)

Event Timeline

mati865 created this revision.Fri, Sep 11, 9:16 AM
mati865 requested review of this revision.Fri, Sep 11, 9:16 AM
mstorsjo added inline comments.Fri, Sep 11, 12:09 PM
llvm/tools/llvm-config/llvm-config.cpp
382–385

Shouldn't this be isWindowsGNUEnvironment()?

mati865 added inline comments.Fri, Sep 11, 1:16 PM
llvm/tools/llvm-config/llvm-config.cpp
382–385

Sorry, I did not get along with my editor when making last second changes.
I'll update diff later when builds finish.

mati865 updated this revision to Diff 291348.Fri, Sep 11, 3:14 PM
mati865 marked an inline comment as done.

If the corresponding DLLs built in MSVC mode actually have a version number suffix, should we change the build for MinGW mode to include a number as well? Or is any of the relevant DLLs ever built in MSVC mode at all?

mati865 updated this revision to Diff 291379.Sat, Sep 12, 1:48 AM

Right.. but, put another way - what code appends a version number in dynamic libs when built for other platforms, but decides not to, when targeting windows in general, or mingw specifically? (If there's a mingw-specific clause here, there should be a corresponding one in the cmakefiles somewhere - but I'm wondering if the right course of action would be to remove that mingw specific clause in cmakefiles, instead of adding a matching one here?)

mati865 added a comment.EditedSat, Sep 12, 6:23 AM

what code appends a version number in dynamic libs when built for other platforms, but decides not to, when targeting windows in general, or mingw specifically?

I have very limited knowledge about what is going on but I think this version script controls it: https://github.com/llvm/llvm-project/blob/9c651c231f3144f53e13cd0a1747589e1b2edccd/llvm/tools/llvm-shlib/CMakeLists.txt#L51

what code appends a version number in dynamic libs when built for other platforms, but decides not to, when targeting windows in general, or mingw specifically?

I have very limited knowledge about what is going on but I think this version script controls it: https://github.com/llvm/llvm-project/blob/9c651c231f3144f53e13cd0a1747589e1b2edccd/llvm/tools/llvm-shlib/CMakeLists.txt#L51

The version script only affects how symbols are exported, but I can try to dig around to see where it comes from in the filename.

mstorsjo added a subscriber: hans.

Ok, so if I read code correctly, the suffix gets added here: https://github.com/llvm/llvm-project/blob/9c651c231f3144f53e13cd0a1747589e1b2edccd/llvm/cmake/modules/AddLLVM.cmake#L599-L606

So it gets added within an if (UNIX) condition. Hence one shouldn't expect version suffix on DLLs built for windows, regardless of MSVC vs mingw. The prebuilt installers of LLVM for windows (targeting MSVC) would confirm this; they contain LLVM-C.dll and LTO.dll.

However the code here in llvm-config seems to add LLVM_DYLIB_VERSION in the filenames, regardless of that logic. So the right path forward, if I understand things correctly, would be to stop adding LLVM_DYLIB_VERSION into the expected filenames for any windows target, not only mingw.

I had an old msvc based build lying around, and there I see the same, I have unsuffixed DLLs, and if trying to run e.g. llvm-config.exe --link-shared I'm getting llvm-config: error: LLVM-10svn.dll is missing. So this change, if applied to MSVC configurations, would be correct there as well.

@hans - Does that sound right to you?

Cygwin seems to be the only Windows target that adds version to the library names.

Cygwin seems to be the only Windows target that adds version to the library names.

Right - I guess cmake treats UNIX as true when targeting cygwin.

hans added a comment.Mon, Sep 14, 5:26 AM

@hans - Does that sound right to you?

Yes, that matches my experience. But I don't know the backstory here. Maybe it would make sense to have version suffixes also on Windows?

@hans - Does that sound right to you?

Yes, that matches my experience. But I don't know the backstory here. Maybe it would make sense to have version suffixes also on Windows?

I guess it would make sense - I don't see why it shouldn't be used there as well. There's probably some other unix-isms within the same if (UNIX block in cmake, but at least the version number aspect probably would make sense to use on windows on both mingw and msvc configurations.

mati865 abandoned this revision.Tue, Sep 22, 3:08 PM