Page MenuHomePhabricator

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

Authored by zahiraam on Jul 17 2020, 8:54 AM.

Diff Detail

Event Timeline

zahiraam created this revision.Jul 17 2020, 8:54 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
jdoerfert resigned from this revision.Jul 17 2020, 9:21 AM
aaron.ballman added inline comments.Jul 18 2020, 9:32 AM
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 edited reviewers, added: majnemer; removed: jdoerfert.Jul 18 2020, 9:33 AM
zahiraam updated this revision to Diff 279103.Jul 19 2020, 11:45 AM
zahiraam marked 2 inline comments as done.
aaron.ballman added a subscriber: beanz.

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?

zahiraam updated this revision to Diff 279602.Jul 21 2020, 12:01 PM
beanz added a subscriber: rnk.Jul 21 2020, 12:59 PM

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.

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

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.

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

aeubanks added a subscriber: aeubanks.

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 updated this revision to Diff 279795.Jul 22 2020, 6:18 AM

Review please? Thanks.

beanz accepted this revision.Jul 27 2020, 3:33 PM

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.

This revision is now accepted and ready to land.Jul 27 2020, 3:33 PM
aaron.ballman accepted this revision.Jul 27 2020, 4:07 PM

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

Meinersbur added inline comments.
llvm/cmake/modules/HandleLLVMOptions.cmake
476

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,
477

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 updated this revision to Diff 281205.Jul 28 2020, 5:54 AM
zahiraam retitled this revision from Passing the flag bigobj globally when building debug compiler on Windows to On Windows build, making the /bigobj flag global , instead of passing it per file..
zahiraam marked an inline comment as done.
zahiraam added inline comments.
llvm/cmake/modules/HandleLLVMOptions.cmake
477

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

Meinersbur accepted this revision.Jul 28 2020, 10:14 AM
Meinersbur added inline comments.
llvm/cmake/modules/HandleLLVMOptions.cmake
477

👍

Can someone submit it please? Thanks.

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 added a comment.EditedJul 28 2020, 12:27 PM

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.

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.

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 28 2020, 4:05 PM