Closed Bug 1479800 Opened 6 years ago Closed 6 years ago

We need a way to build clang_rt.profile-i386.lib

Categories

(Firefox Build System :: Toolchains, enhancement)

enhancement
Not set
normal

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: away, Assigned: glandium)

References

Details

Attachments

(1 file)

Doing PGO on 32-bit clang-cl builds requires that we link against clang_rt.profile-i386.lib during the instrumentation phase. LLVM's build system only produces this library when you build a 32-bit clang-cl. (I asked, and it's on their list to improve this, but currently this is what we have to deal with.)

So we'll need to reintroduce 32-bit clang-cl builds to some extent. I don't think we want to use the actual 32-bit compiler because I'm pretty sure it'll OOM during LTO. Likely the best approach is to build only the profiling library and use the 64-bit toolchain otherwise. The 32-bit piece ought to be a very lightweight build, judging by:

D:\llvmTest32>ninja -n clang_rt.profile-i386.lib
[17/17] Linking CXX static library lib\clang\7.0.0\lib\windows\clang_rt.profile-i386.lib

I don't know if it will be a problem for our build environment to get files from multiple clang packages.
(In reply to David Major [:dmajor] from comment #0)
> I don't know if it will be a problem for our build environment to get files
> from multiple clang packages.

I want to say that we already do this for the fuzzing/asan clang builds, because they need linux libraries and mac libraries?
See Also: → 1399396
(In reply to Nathan Froyd [:froydnj] from comment #1)
> I want to say that we already do this for the fuzzing/asan clang builds,
> because they need linux libraries and mac libraries?

Yeah, that was bug 1425406. I filed bug 1421755 on fixing our build-clang machinery to do this in a smarter way. The clang CMake build system supports building multiple targets in one pass nowadays, so we ought to be able to do this in a supported way.
See Also: → 1421755
So, I was looking at the same thing for x86 linux, and there's actually something interesting going on, where the stage1 *does* have x86 and x86-64 libclang_rt libraries, but not stage2 or stage3.
So the difference is that when compiling clang with gcc, cmake says:
Compiler-RT supported architectures: x86_64;i386
(...)
Builtin supported architectures: i386;x86_64

but when building clang with clang in subsequent stages,it says:
Compiler-RT supported architectures: x86_64
(...)
Builtin supported architectures: x86_64
So the problem for linux is that clang can't build 32-bits binaries during stage 2 and 3 because it can't find libgcc_s.so.

For windows, it would seem to be a different issue: it looks like from the log that it doesn't even try to build a 32-bits compiler-rt. It would be good to check why the __i386__ test doesn't happen at all on Windows.
This is not trivially fixable on Windows.

The root of the problem is, per comment 4, the compiler-rt check for targets.

The check is here: https://github.com/llvm-mirror/compiler-rt/blob/0bf1004a1492af7a923ae8051dcfd1b9c8eb6c9e/cmake/base-config-ix.cmake#L155-L172

Let's not care about stage 1, but it's worth noting stage 1 is compiled with MSVC and falls in the MSVC case, so it never ever tries to build for targets.
Now, in stage 2, we're building with clang-cl. But that MSVC symbol is still true because that's what cmake decides. So we fall in the same code path as MSVC.

That part is easily fixable, and I did so, with something like the following:
diff --git a/cmake/base-config-ix.cmake b/cmake/base-config-ix.cmake
index 91fe2494b..77134e1a5 100644
--- a/cmake/base-config-ix.cmake
+++ b/cmake/base-config-ix.cmake
@@ -152,7 +152,7 @@ macro(test_targets)
     if(COMPILER_RT_DEFAULT_TARGET_ONLY)
       add_default_target_arch(${COMPILER_RT_DEFAULT_TARGET_ARCH})
     elseif("${COMPILER_RT_DEFAULT_TARGET_ARCH}" MATCHES "i[2-6]86|x86|amd64")
-      if(NOT MSVC)
+      if(NOT MSVC OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
         if(CMAKE_SYSTEM_NAME MATCHES "OpenBSD")
           if (CMAKE_SIZEOF_VOID_P EQUAL 4)
             test_target_arch(i386 __i386__ "-m32")

The next hurdle is that the `test_target_arch` test, which uses cmake's check_symbol_exists, does a compilation/linkage cycle, the former with clang-cl, and the latter with... msvc's link.exe.

The problem then is that there's only one LIB set, and even if it contains both x86 and x64 paths, link.exe tries to link the x64 kernel.32.lib and friends, and fails with "warning LNK4272: library machine type 'x64' conflicts with target machine type 'x86'". Putting the x86 lib paths first fails with the opposite message much earlier.

I also tried using lld-link for stage 2, and it failed in the same way.

A linker wrapper that sets LIB appropriately might help here, but it would have to detect which set of lib paths should be used, and there's nothing explicit on the command line indicating so (-m32 is not passed to the linker). One would need to parse the command line to get the object files, and find out if the object files are 32 or 64 bits...
At this point would it be easier to just have an additional 32-bit build-clang task that builds only this one target, like in comment 0?
How bad of an idea would it be to just check this lib into the tree? (182K at MinSizeRel)
Not really great. I'm going to try to pass the lib paths on the command line instead of with the LIB environment variable, and see if I can go further this way.
Assignee: nobody → mh+mozilla
Blocks: 1483779
Attachment #9001557 - Flags: review?(core-build-config-reviews)
Comment on attachment 9001557 [details] [diff] [review]
Build 32-bits compiler-rt for 64-bits clang-cl

Review of attachment 9001557 [details] [diff] [review]:
-----------------------------------------------------------------

I feel kinda gross about putting such purpose-specific hard-coding into build-clang.py. But I suppose it does the job. :/

::: build/build-clang/build-clang.py
@@ +219,5 @@
> +        return cmake_args
> +
> +    cmake_args = cmake_base_args(
> +        cc, cxx, asm, ld, ar, ranlib, libtool, inst_dir)
> +    cmake_args += [

Can you share the reasoning for the additional args being left out of cmake_base_args? I wonder if we could keep more of them in the base? They should be harmless for compiler-rt. Unless its cmake errors out over unrecognized defines? (If that's the case, maybe a brief comment would help.)

@@ +260,5 @@
> +            # Only build the 32-bits compiler-rt when we originally built for
> +            # 64-bits, which we detect through the contents of the LIB
> +            # environment variable, which we also adjust for a 32-bits build
> +            # at the same time.
> +            lib = os.environ['LIB']

Maybe clarify that this is an old_lib or saved_lib.
Attachment #9001557 - Flags: review?(core-build-config-reviews) → review+
(In reply to David Major [:dmajor] from comment #11)
> Comment on attachment 9001557 [details] [diff] [review]
> Build 32-bits compiler-rt for 64-bits clang-cl
> 
> Review of attachment 9001557 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I feel kinda gross about putting such purpose-specific hard-coding into
> build-clang.py. But I suppose it does the job. :/

The ideal solution is something akin to what is suggested in bug 1421755, but the compiler-rt cmake scripts don't allow to easily build for multiple targets with msvc/clang-cl. Part of the problem being our setup using the LIB environment variable (we should start switching to the -LIBPATH command line option instead), another part being that the compiler-rt cmake script doesn't allow per target flags. Maybe in a more "do everything with cmake" world it would work out fine, with a separate build of compiler-rt per target? Anyways, that's much more effort than I'm ready to put in for this.

> ::: build/build-clang/build-clang.py
> @@ +219,5 @@
> > +        return cmake_args
> > +
> > +    cmake_args = cmake_base_args(
> > +        cc, cxx, asm, ld, ar, ranlib, libtool, inst_dir)
> > +    cmake_args += [
> 
> Can you share the reasoning for the additional args being left out of
> cmake_base_args?

No particular reason besides avoiding to pass useless flags.
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e71e61d1623
Build 32-bits compiler-rt for 64-bits clang-cl. r=dmajor
Backout by mh@glandium.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/55e5eedb90cb
Backout changeset 1e71e61d1623 to give time to toolchains to build without blocking other landings.
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dccd5299c5ad
Build 32-bits compiler-rt for 64-bits clang-cl. r=dmajor
Pushed by aiakab@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ac9bb47e988
Build 32-bits compiler-rt for 64-bits clang-cl. r=dmajor
Backout by aiakab@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8adbf514581c
Backed out 2 changesets (bug 1479800, bug 1483779)for frequent cgx and arm64 failures
https://hg.mozilla.org/mozilla-central/rev/6ac9bb47e988
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.