On Windows build, making the /bigobj flag global , instead of passing it per file.

[zahiraam]

Summary

To avoid having this flag be passed in per/file manner, we are instead passing it globally.
This fixes this bug: https://bugs.llvm.org/show_bug.cgi?id=46733

[aaron.ballman]

clang/CMakeLists.txt

417 ↗

(On Diff #278791)

Why only for debug builds instead of all builds?

418 ↗

(On Diff #278791)

Do we really need to force synchronous generation of PDB files? This bit from the docs worries me "This may make builds significantly longer, and it doesn't prevent all errors that may occur when multiple instances of cl.exe access the PDB file at the same time."

llvm/CMakeLists.txt

604 ↗

(On Diff #278791)

Same question here about debug builds.

[aaron.ballman]

I think this is missing changes to clang\lib\ARCMigrate\CMakeLists.txt(8): set_source_files_properties(Transforms.cpp PROPERTIES COMPILE_FLAGS /bigobj).

clang/CMakeLists.txt

416 ↗

(On Diff #279103)

Is this change actually needed? I would have assumed that Clang was inheriting the compile options from LLVM automatically?

llvm/CMakeLists.txt

603 ↗

(On Diff #279103)

I would expect to find these changes in HandleLLVMOptions.cmake somewhere around line 390 (where we're setting up the other global MSVC flags), but I'm not the expert in our CMake. @beanz, what say you?

[beanz]

Sadly I think @rnk isn't around. This is something I'd really like him to weigh in on. I assume there was a reason for doing this per-object file rather than for everything, and I don't have the context.

llvm/CMakeLists.txt

603 ↗

(On Diff #279103)

Yep. This should definitely be in HandleLLVMOptions.cmake, not the root CMakeLists.txt.

[zahiraam]

In D84038#2165110, @beanz wrote:

Sadly I think @rnk isn't around. This is something I'd really like him to weigh in on. I assume there was a reason for doing this per-object file rather than for everything, and I don't have the context.

@beanz There is more context here: https://bugs.llvm.org/show_bug.cgi?id=46733

[aaron.ballman]

In D84038#2165110, @beanz wrote:

Sadly I think @rnk isn't around. This is something I'd really like him to weigh in on. I assume there was a reason for doing this per-object file rather than for everything, and I don't have the context.

I can provide some historical context about why we did this per-object initially -- it's because I argued for it to be that way. :-D My argument was that needing this flag was a bad code smell for what usually was a template instantiation issue that could be solved in code and if we enabled this flag for all files, we may hide some rampant template instantiation issues. I've since become convinced that the cure to those instantiation issues is sometimes worse than the symptom and we will deal with the truly problematic compilation units when we audit build performance issues or notice a compilation unit size regression.

[aaron.ballman]

I knew if I searched hard enough, I'd find it; historical context: http://lists.llvm.org/pipermail/cfe-dev/2014-August/038885.html

[amccarth]

Just a few days ago, I landed another single file /bigobj to get some LLDB tests running: https://reviews.llvm.org/rG14dde438d69c81ab4651157a94d32e5555e804ff

I did it as a single-file change in order to not perturb things more than necessary for my own use case.

I don't think there's a problem applying the change globally. I think past concerns were:

• Compatibility: Old linkers (ca 2005) couldn't read /BIGOBJ files, but those days are long behind us.
• Performance: The bug report shows the link-time impact is pretty small (about 1% for LLVM).
• Object size: Each object file would be slightly larger. I think the discussion on the bug report puts it on the order of kilobytes. That's not a lot, unless you have many, many object files.

Arguably, hitting the smaller limit could be an early warning that a file has gotten out of control (e.g., with excessive template instantiations). On the other hand, making individuals figure this out each time a file crosses the threshold has a non-zero development cost.

Applying it just to debug builds may be enough. Debug builds produce more sections. I lost the reference, but just the other day I read each externally visible symbol can result in 4 or more sections in a debug build, but only 2 or 3 in a non-debug build. So 64-ish ksections is easier to hit in a debug build.

clang-cl ignores /BIGOBJ. There are comments that it handles /bigobj "automatically," which I assume means it switches to COFF bigobj if the threshold is hit.

I don't think there's much risk to this proposal, especially if we limit to debug builds, but I'd support it for all builds if that's the consensus.

The associated bug report shows only a small impact on link time, so I don't think that's a real concern. Object file size increases are individually small, but I guess for some all of them together might add up.

[zahiraam]

Review please? Thanks.

[beanz]

The CMake code looks good to me. If @aaron.ballman is on board with it from the Windows perspective it sounds like this is the right fix.

[aaron.ballman]

This LGTM as well. Thank you for working on this!

[Meinersbur]

llvm/cmake/modules/HandleLLVMOptions.cmake

471

This comment belongs into the commit description; readers of this file will not see what the previous state was. Maybe explain here why /bigobj is needed, as by the lines removed:

# By default MSVC has a 2^16 limit on the number of sections in an object file,

472

Consider add_compile_options instead? CMAKE_CXX_FLAGS is meant to be defined by the user. However, since the rest of the file also appends to CMAKE_CXX_FLAGS, might also leave it consistent with the others.

[zahiraam]

llvm/cmake/modules/HandleLLVMOptions.cmake

472

Opted to be consistent and use the append instead. OK?

[Meinersbur]

llvm/cmake/modules/HandleLLVMOptions.cmake

472

👍

[zahiraam]

Can someone submit it please? Thanks.

[zahiraam]

I actually made an attempt to commit the patch using arc but not sure it made it. This was my last command: git commit --amend -a
Do I need to push now?
Thanks.

[mehdi_amini]

In D84038#2179551, @zahiraam wrote:

I actually made an attempt to commit the patch using arc but not sure it made it. This was my last command: git commit --amend -a
Do I need to push now?
Thanks.

You can use arc land --revision D84038 or you can check what you're about to push: git fetch origin && git log ^origin/master HEAD ; if this is the commit you intend to push and the commit message is satisfying, then just push it.

If you don't have push permissions, you need to ask here for someone to land it for you.

[zahiraam]

In D84038#2179734, @mehdi_amini wrote:

In D84038#2179551, @zahiraam wrote:

I actually made an attempt to commit the patch using arc but not sure it made it. This was my last command: git commit --amend -a
Do I need to push now?
Thanks.

You can use arc land --revision D84038 or you can check what you're about to push: git fetch origin && git log ^origin/master HEAD ; if this is the commit you intend to push and the commit message is satisfying, then just push it.

If you don't have push permissions, you need to ask here for someone to land it for you.

Can someone land it please? Thanks.

clang/lib/ASTMatchers/Dynamic/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
 )
 
-# The registry source file ends up generating a lot of sections for each
-# matcher. Each matcher appears to get a vtable and several methods. Each
-# method needs .text, .pdata, .xdata, and .debug sections, adding to the
-# section multiplier. By default MSVC has a 2^16 limit on the number of
-# sections in an object file, and this needs more than that.
-if (MSVC)
-  set_source_files_properties(Registry.cpp PROPERTIES COMPILE_FLAGS /bigobj)
-endif()
-
 add_clang_library(clangDynamicASTMatchers
   Diagnostics.cpp
   Marshallers.cpp
   Parser.cpp
   Registry.cpp
   VariantValue.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
 
   DEPENDS
   omp_gen
   )

clang/lib/CodeGen/CMakeLists.txt

 # building codegen. In a standalone build, LLVM is already built and we don't
 # need this dependency. Furthermore, LLVM doesn't export it so we can't have
 # this dependency.
 set(codegen_deps intrinsics_gen)
 if (CLANG_BUILT_STANDALONE)
   set(codegen_deps)
 endif()
 
-if (MSVC)
-  set_source_files_properties(CodeGenModule.cpp PROPERTIES COMPILE_FLAGS /bigobj)
-endif()
-
 add_clang_library(clangCodeGen
   BackendUtil.cpp
   CGAtomic.cpp
   CGBlocks.cpp
   CGBuiltin.cpp
   CGCUDANV.cpp
   CGCUDARuntime.cpp
   CGCXX.cpp

clang/lib/Sema/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
-if (MSVC)
-  set_source_files_properties(SemaDeclAttr.cpp PROPERTIES COMPILE_FLAGS /bigobj)
-  set_source_files_properties(SemaExpr.cpp PROPERTIES COMPILE_FLAGS /bigobj)
-  set_source_files_properties(SemaExprCXX.cpp PROPERTIES COMPILE_FLAGS /bigobj)
-  set_source_files_properties(SemaTemplate.cpp PROPERTIES COMPILE_FLAGS /bigobj)
-  set_source_files_properties(SemaTemplateDeduction.cpp PROPERTIES COMPILE_FLAGS /bigobj)
-  set_source_files_properties(SemaOpenMP.cpp PROPERTIES COMPILE_FLAGS /bigobj)
-endif()
-
 clang_tablegen(OpenCLBuiltins.inc -gen-clang-opencl-builtins
   SOURCE OpenCLBuiltins.td
   TARGET ClangOpenCLBuiltinsImpl
   )
 
 add_clang_library(clangSema
   AnalysisBasedWarnings.cpp
   CodeCompleteConsumer.cpp

clang/unittests/AST/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
-if (MSVC)
-  set_source_files_properties(ASTImporterTest.cpp PROPERTIES COMPILE_FLAGS /bigobj)
-endif()
 
 add_clang_unittest(ASTTests
   ASTContextParentMapTest.cpp
   ASTImporterFixtures.cpp
   ASTImporterTest.cpp
   ASTImporterGenericRedeclTest.cpp
   ASTImporterODRStrategiesTest.cpp
   ASTImporterVisibilityTest.cpp

clang/unittests/ASTMatchers/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
-# By default MSVC has a 2^16 limit on the number of sections in an object file,
-# and this needs more than that.
-if (MSVC)
-  set_source_files_properties(InternalASTMatchersTest.cpp PROPERTIES COMPILE_FLAGS /bigobj)
-  set_source_files_properties(NodeMatchersTest.cpp PROPERTIES COMPILE_FLAGS /bigobj)
-  set_source_files_properties(NarrowingMatchersTest.cpp PROPERTIES COMPILE_FLAGS /bigobj)
-  set_source_files_properties(ASTTraversalMatchersTest.cpp PROPERTIES COMPILE_FLAGS /bigobj)
-endif()
-
 add_clang_unittest(ASTMatchersTests
   ASTMatchersInternalTest.cpp
   ASTMatchersNodeTest.cpp
   ASTMatchersNarrowingTest.cpp
   ASTMatchersTraversalTest.cpp
   GtestMatchersTest.cpp
   )
 

clang/unittests/Tooling/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   ${LLVM_TARGETS_TO_BUILD}
   FrontendOpenMP
   Support
   )
 
-# By default MSVC has a 2^16 limit on the number of sections in an object file,
-# and this needs more than that.
-if (MSVC)
-  set_source_files_properties(RecursiveASTVisitorTest.cpp PROPERTIES COMPILE_FLAGS /bigobj)
-  set_source_files_properties(RecursiveASTVisitorTestExprVisitor.cpp PROPERTIES COMPILE_FLAGS /bigobj)
-  set_source_files_properties(RecursiveASTVisitorTests/Callbacks.cpp PROPERTIES COMPILE_FLAGS /bigobj)
-  set_source_files_properties(SourceCodeTest.cpp PROPERTIES COMPILE_FLAGS /bigobj)
-endif()
 
 add_clang_unittest(ToolingTests
   ASTSelectionTest.cpp
   CastExprTest.cpp
   CommentHandlerTest.cpp
   CompilationDatabaseTest.cpp
   DependencyScannerTest.cpp
   DiagnosticsYamlTest.cpp

lldb/source/API/CMakeLists.txt

if(LLDB_ENABLE_PYTHON AND (BUILD_SHARED_LIBS OR LLVM_LINK_LLVM_DYLIB) AND UNIX AND NOT APPLE)
   set_property(TARGET liblldb APPEND PROPERTY INSTALL_RPATH "\$ORIGIN/../../../../lib${LLVM_LIBDIR_SUFFIX}")
 endif()
 
 if(PYTHON_RPATH)
   set_property(TARGET liblldb APPEND PROPERTY INSTALL_RPATH "${PYTHON_RPATH}")
   set_property(TARGET liblldb APPEND PROPERTY BUILD_RPATH   "${PYTHON_RPATH}")
 endif()
 
-if (MSVC)
-  set_source_files_properties(SBReproducer.cpp PROPERTIES COMPILE_FLAGS /bigobj)
-endif()
 
 if(lldb_python_wrapper)
   add_dependencies(liblldb swig_wrapper)
 
   if (MSVC)
     set_property(SOURCE ${lldb_python_wrapper} APPEND_STRING PROPERTY COMPILE_FLAGS " /W0")
   else()
     set_property(SOURCE ${lldb_python_wrapper} APPEND_STRING PROPERTY COMPILE_FLAGS " -w")

llvm/cmake/modules/HandleLLVMOptions.cmake

if (SUPPORTS_BREPRO)
 
       if (${linker_flag_idx} GREATER -1)
         message(WARNING "/Brepro not compatible with /INCREMENTAL linking - builds will be non-deterministic")
       else()
         append("/Brepro" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
       endif()
     endif()
   endif()
+  # Making this flag global instead of per file.
+  append("/bigobj" CMAKE_CXX_FLAGS)
 endif( MSVC )
 
 # Warnings-as-errors handling for GCC-compatible compilers:
 if ( LLVM_COMPILER_IS_GCC_COMPATIBLE )
   append_if(LLVM_ENABLE_WERROR "-Werror" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
   append_if(LLVM_ENABLE_WERROR "-Wno-error" CMAKE_REQUIRED_FLAGS)
 endif( LLVM_COMPILER_IS_GCC_COMPATIBLE )
 

llvm/lib/Passes/CMakeLists.txt

-if (MSVC)
-  set_source_files_properties(PassBuilder.cpp PROPERTIES COMPILE_FLAGS /bigobj)
-endif()
-
 add_llvm_component_library(LLVMPasses
   PassBuilder.cpp
   PassPlugin.cpp
   StandardInstrumentations.cpp
 
   ADDITIONAL_HEADER_DIRS
   ${LLVM_MAIN_INCLUDE_DIR}/llvm
   ${LLVM_MAIN_INCLUDE_DIR}/llvm/Passes
 
   DEPENDS
   intrinsics_gen
   )

mlir/lib/Dialect/SPIRV/CMakeLists.txt

-if (MSVC)
-  set_source_files_properties(SPIRVDialect.cpp PROPERTIES COMPILE_FLAGS /bigobj)
-endif()
 
 set(LLVM_TARGET_DEFINITIONS SPIRVCanonicalization.td)
 mlir_tablegen(SPIRVCanonicalization.inc -gen-rewriters)
 add_public_tablegen_target(MLIRSPIRVCanonicalizationIncGen)
 
 add_mlir_dialect_library(MLIRSPIRV
   LayoutUtils.cpp
   SPIRVAttributes.cpp