Incremental build bug in Windows exe manifests

[Reilly Grant]

I am looking into a Windows build determinism bug which appears to come down to an incremental build issue which was recently triggered by a change to the default Windows executable manifest: https://bugs.chromium.org/p/chromium/issues/detail?id=1285802

I'm looking at the definition of //build/win:default_exe_manifest and the "windows_manifest" GN template and I'm assuming the issue is that the GN rules don't trigger re-linking of executables when the manifest changes. As far as I can tell this might be because the manifest selection is based on ldflags and so there isn't a real dependency created between the executable target and the manifest file. I tried modifying the "windows_manifest" template to add invoker.sources as inputs to the source_set it creates but that doesn't seem to have any effect. I'm out of my depth in terms of understanding how GN targets work.

Reilly Grant Software Engineer rei...@google.com Google Chrome

[Brett Wilson]

From my recollection and from the code, it looks like the "inputs" should trigger a re-link.

The way to debug this kind of thing is to debug the Ninja commands. The syntax is quite easy, the hard part is finding the right file. It should be a ".ninja" file in out/FOO/obj/<the same directory> with the same name as the original target (watch out, various things with chrome.exe/dll are renamed so maybe just grep).

I looked at the code and this is suspicious:

https://gn.googlesource.com/gn/+/refs/heads/main/src/gn/ninja_c_binary_target_writer.cc#773

"The input dependency is only needed if there are no object files, as the dependency is normally provided transitively by the source files."

So according to this the .obj files should have a dependency on your manifest and that should in turn trigger a re-link. It's possible this doesn't apply to your case and this is a bug. Like maybe the .obj files aren't touched if they don't change or there aren't any .obj files in this particular target.

Brett

[Reilly Grant]

Here's out\Default\obj\third_party\catapult\telemetry\bitmaptools.ninja, which is affected by this issue:

defines = -DUSE_AURA=1 "-DCR_CLANG_REVISION=\"llvmorg-14-init-12719-gc4b45eeb-3\"" -D_HAS_NODISCARD -D_CRT_NONSTDC_NO_WARNINGS -D_WINSOCK_DEPRECATED_NO_WARNINGS -D_LIBCPP_ABI_UNSTABLE -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -D_LIBCPP_ENABLE_NODISCARD -D_LIBCPP_NO_AUTO_LINK -D__STD_C -D_CRT_RAND_S -D_CRT_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_DEPRECATE -D_ATL_NO_OPENGL -D_WINDOWS -DCERT_CHAIN_PARA_HAS_EXTRA_FIELDS -DPSAPI_VERSION=2 -DWIN32 -D_SECURE_ATL -DWINAPI_FAMILY=WINAPI_FAMILY_DESKTOP_APP -DWIN32_LEAN_AND_MEAN -DNOMINMAX -D_UNICODE -DUNICODE -DNTDDI_VERSION=NTDDI_WIN10_VB -D_WIN32_WINNT=0x0A00 -DWINVER=0x0A00 -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0
include_dirs = -I../.. -Igen -I../../buildtools/third_party/libc++
cflags = -fno-delete-null-pointer-checks -fno-ident -fcolor-diagnostics -fmerge-all-constants -fcrash-diagnostics-dir=../../tools/clang/crashreports -mllvm -instcombine-lower-dbg-declare=0 /clang$:-ffp-contract=off -fcomplete-member-pointers /Gy /FS /bigobj /utf-8 /Zc$:twoPhase /Zc$:sizedDealloc- /D__WRL_ENABLE_FUNCTION_STATICS__ -fmsc-version=1916 -m32 -msse3 /Brepro -Wno-builtin-macro-redefined -D__DATE__= -D__TIME__= -D__TIMESTAMP__= -ffile-compilation-dir=. -no-canonical-prefixes /W4 -Wimplicit-fallthrough -Wunreachable-code-aggressive -Wthread-safety -Wextra-semi /WX -Wno-missing-field-initializers -Wno-unused-parameter -Wloop-analysis -Wno-unneeded-internal-declaration -Wno-nonportable-include-path -Wno-null-pointer-subtraction -Wenum-compare-conditional -Wno-psabi -Wno-ignored-pragma-optimize -Wmax-tokens -Wshadow /O1 /Ob2 /Oy- /Zc$:inline /Gw /Oi -gcodeview-ghash -gline-tables-only -ftrivial-auto-var-init=pattern /guard$:cf /MT -Xclang -add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs -Xclang raw-ptr-template-as-trivial-member -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare
cflags_cc = /std$:c++17 -Wno-trigraphs /Zc$:alignedNew- /TP /GR- -I../../buildtools/third_party/libc++/trunk/include
label_name = bitmaptools
target_out_dir = obj/third_party/catapult/telemetry
target_output_name = bitmaptools

build obj/third_party/catapult/telemetry/bitmaptools/bitmaptools.obj: cxx ../../third_party/catapult/telemetry/telemetry/internal/image_processing/bitmaptools.cc

build ./bitmaptools.exe ./bitmaptools.exe.pdb: link obj/third_party/catapult/telemetry/bitmaptools/bitmaptools.obj obj/buildtools/third_party/libc++/libc++/algorithm.obj obj/buildtools/third_party/libc++/libc++/any.obj obj/buildtools/third_party/libc++/libc++/atomic.obj obj/buildtools/third_party/libc++/libc++/barrier.obj obj/buildtools/third_party/libc++/libc++/bind.obj obj/buildtools/third_party/libc++/libc++/charconv.obj obj/buildtools/third_party/libc++/libc++/chrono.obj obj/buildtools/third_party/libc++/libc++/condition_variable.obj obj/buildtools/third_party/libc++/libc++/condition_variable_destructor.obj obj/buildtools/third_party/libc++/libc++/debug.obj obj/buildtools/third_party/libc++/libc++/exception.obj obj/buildtools/third_party/libc++/libc++/functional.obj obj/buildtools/third_party/libc++/libc++/future.obj obj/buildtools/third_party/libc++/libc++/hash.obj obj/buildtools/third_party/libc++/libc++/ios.obj obj/buildtools/third_party/libc++/libc++/ios.instantiations.obj obj/buildtools/third_party/libc++/libc++/iostream.obj obj/buildtools/third_party/libc++/libc++/locale.obj obj/buildtools/third_party/libc++/libc++/memory.obj obj/buildtools/third_party/libc++/libc++/mutex.obj obj/buildtools/third_party/libc++/libc++/mutex_destructor.obj obj/buildtools/third_party/libc++/libc++/new.obj obj/buildtools/third_party/libc++/libc++/optional.obj obj/buildtools/third_party/libc++/libc++/random.obj obj/buildtools/third_party/libc++/libc++/random_shuffle.obj obj/buildtools/third_party/libc++/libc++/regex.obj obj/buildtools/third_party/libc++/libc++/shared_mutex.obj obj/buildtools/third_party/libc++/libc++/stdexcept.obj obj/buildtools/third_party/libc++/libc++/string.obj obj/buildtools/third_party/libc++/libc++/strstream.obj obj/buildtools/third_party/libc++/libc++/system_error.obj obj/buildtools/third_party/libc++/libc++/thread.obj obj/buildtools/third_party/libc++/libc++/typeinfo.obj obj/buildtools/third_party/libc++/libc++/utility.obj obj/buildtools/third_party/libc++/libc++/valarray.obj obj/buildtools/third_party/libc++/libc++/variant.obj obj/buildtools/third_party/libc++/libc++/vector.obj obj/buildtools/third_party/libc++/libc++/locale_win32.obj obj/buildtools/third_party/libc++/libc++/support.obj obj/buildtools/third_party/libc++/libc++/thread_win32.obj || obj/build/win/default_exe_manifest.stamp obj/build/config/executable_deps.stamp obj/buildtools/third_party/libc++/libc++.stamp
  ldflags = --color-diagnostics /call-graph-profile-sort$:no /TIMESTAMP$:1641099600 /lldignoreenv /OPT$:REF /OPT$:ICF /INCREMENTAL$:NO /FIXED$:NO /OPT$:NOLLDTAILMERGE /PROFILE /PDBSourcePath$:o$:\fake\prefix /WX /OPT$:ICF /DEBUG$:GHASH /pdbaltpath$:%_PDB% /NATVIS$:../../build/config/c++/libc++.natvis /DEFAULTLIB$:libcpmt.lib /MACHINE$:X86 /SAFESEH /largeaddressaware /FIXED$:NO /ignore$:4199 /ignore$:4221 /NXCOMPAT /DYNAMICBASE /INCREMENTAL$:NO /SUBSYSTEM$:CONSOLE,5.01 /guard$:cf /manifest$:embed /manifestuac$:no /manifestinput$:../../build/win/as_invoker.manifest /manifestinput$:../../build/win/common_controls.manifest /manifestinput$:../../build/win/compatibility.manifest /LIBPATH:"../../third_party/depot_tools/win_toolchain/vs_files/3bda71a11e/Windows$ Kits/10/Lib/10.0.19041.0/um/x86" /LIBPATH:../../third_party/depot_tools/win_toolchain/vs_files/3bda71a11e/VC/Tools/MSVC/14.26.28801/lib/x86 /LIBPATH:../../third_party/depot_tools/win_toolchain/vs_files/3bda71a11e/VC/Tools/MSVC/14.26.28801/atlmfc/lib/x86
  libs = advapi32.lib comdlg32.lib dbghelp.lib dnsapi.lib gdi32.lib msimg32.lib odbc32.lib odbccp32.lib oleaut32.lib shell32.lib shlwapi.lib user32.lib usp10.lib uuid.lib version.lib wininet.lib winmm.lib winspool.lib ws2_32.lib delayimp.lib kernel32.lib ole32.lib
  frameworks =
  swiftmodules =
  output_extension = .exe
  output_dir = .

You can see the reference to //build/win/compatibility.manifest in the ldflags but it isn't listed in the dependencies for the target. What I've tried doing is adding "inputs = invoker.sources" to build/config/win/manifest.gni on line 105 but it doesn't have any effect on the generated ninja file for this target.

Basically it seems like we need a way to tell ninja that the parameter in ldflags is a file and thus changes to that file should trigger a rebuild. "inputs" seems to be that way but I don't understand exactly how the embedded source_set in the "windows_manifest" template works to affect how that configuration is put in place and now it can create dependencies for the executable target.

Reilly Grant Software Engineer rei...@google.com Google Chrome

[Reilly Grant]

Digging a little deeper it seems like adding the "inputs" declaration does get us part way there. In out\Default\obj\build\win\default_exe_manifest.ninja we now have:

defines = -DUSE_AURA=1 "-DCR_CLANG_REVISION=\"llvmorg-14-init-12719-gc4b45eeb-3\"" -D_HAS_NODISCARD -D_CRT_NONSTDC_NO_WARNINGS -D_WINSOCK_DEPRECATED_NO_WARNINGS -D_LIBCPP_ABI_UNSTABLE -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -D_LIBCPP_ENABLE_NODISCARD -D_LIBCPP_NO_AUTO_LINK -D__STD_C -D_CRT_RAND_S -D_CRT_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_DEPRECATE -D_ATL_NO_OPENGL -D_WINDOWS -DCERT_CHAIN_PARA_HAS_EXTRA_FIELDS -DPSAPI_VERSION=2 -DWIN32 -D_SECURE_ATL -DWINAPI_FAMILY=WINAPI_FAMILY_DESKTOP_APP -DWIN32_LEAN_AND_MEAN -DNOMINMAX -D_UNICODE -DUNICODE -DNTDDI_VERSION=NTDDI_WIN10_VB -D_WIN32_WINNT=0x0A00 -DWINVER=0x0A00 -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0
include_dirs = -I../.. -Igen -I../../buildtools/third_party/libc++

label_name = default_exe_manifest
target_out_dir = obj/build/win
target_output_name = default_exe_manifest

build obj/build/win/default_exe_manifest.inputs.stamp: stamp ../../build/win/as_invoker.manifest ../../build/win/common_controls.manifest ../../build/win/compatibility.manifest

build obj/build/win/default_exe_manifest.stamp: stamp

From the previous message it looks like we do have a dependency on obj/build/win/default_exe_manifest.stamp in the executable target, but no dependency on default_exe_manifest.inputs.stamp which is the one that actually depends on the input files.

Reilly Grant Software Engineer rei...@google.com Google Chrome

[Reilly Grant]

Sorry Brett. I still need some help here. Reading the comment at https://gn.googlesource.com/gn/+/refs/heads/main/src/gn/ninja_c_binary_target_writer.cc#138 it feels like this behavior may be intentional.

Reilly Grant Software Engineer rei...@google.com Google Chrome

[Scott Graham]

Hi Reilly, not sure if you've already resolved this offline...

I think `libs` and `externs` (a Rust thing) are the only `config` variables that are treated as normal file dependencies. I wondered if perhaps it could be hacked by using `libs`, and adding the manifests to that list does declare the dependency causing a build correctly when required. But lld-link doesn't like being told to link a .manifest file (as opposed to having it passed by `/manifestinput:...` (and I couldn't see a way to hack the linker into ignoring the manifest after the fact :).

I can't think of another easy workaround, so my guess is that this will require a GN change.

Other than trying to come up with a sensible name for an inputs-or-sources-like variable that declares more file dependencies (as well as trying to make sure no one uses it in 99% of configs!), another thing to look at might be the handling of `.def` files, which are handled specially in a way that seems like something similar could work for manifest files.

[Nico Weber]

Alternatively, if you just want to get your change in, you could rename the file slightly in the same change in which you change it, then ninja's command-line tracking will force a rebuild.

(Yes, this is a hack.)

[Reilly Grant]

The change in question has already landed. I started looking at this issue as the tree sheriff in order to figure out why the Windows deterministic build bot was red. The bot is now green but I'm guessing that is only because enough other code changes have landed to force a relinking of the executables in question. I've kept myself assigned to the issue in the hope that a fix is possible but it sounds like it needs to be resolved in GN.

Who is a good assignee for an issue like this?

Reilly Grant Software Engineer rei...@google.com Google Chrome

[Roland McGrath]

I believe you can use `inputs` in config() nowadays to propagate a file dependency without affecting the commands run.

[Reilly Grant]

I tried adding `inputs` directly to the config() however GN then complained that the inputs were generated by other targets for which there wasn't a dependency. When I tried to fix that by moving the forward_variables_from(invoker, [ "deps" ]) line from the source_set() to the config() I discovered that config() doesn't support `deps`.

The template I'm talking about is windows_manifest here: https://source.chromium.org/chromium/chromium/src/+/main:build/config/win/manifest.gni;l=76

Reilly Grant Software Engineer rei...@google.com Google Chrome

[Roland McGrath]

Ah yes, that is a problem there.  You can only propagate file dependencies on sources, not on other targets in the build.  The dichotomy between deps and configs as concepts in GN is IMHO the root cause of many subtle kinds of pain.

[Brett Wilson]

Sorry for the delay, I've been distracted with other things for the last couple of days.

Reading this thread I'm not sure where things landed. Inputs aren't propagated, but in general the inputs are effectively propagated because when the executable depends on something else that depends on something else that makes the file, you get the right behavior. There are a few specific cases where this may not apply for the purposes of optimization. On example is for linking shared libraries via their API "toc" files. I don't think these cases apply to you.

Normally if you have a generated manifest I would write it as an action that generates the manifest with an all_dependent_configs that propagates up the linker flag. The normal dependencies on the action should ensure that the action is run before the link. From your description on deps, it seems like maybe this doesn't exist and you have only the resulting config.

As you noticed, you can't add deps on a config. But the above scheme does the reverse where the deps carries the config so they go together.

Brett

[Reilly Grant]

I still feel massively out of my depth here but I think that what you're suggesting (the deps carry the config) is how the windows_manifest template is supposed to work. It creates a source_set and a config and the source_set declares the config in its public_configs. In the case where a manifest is generated I think that will cause the expected relinking but in the case where the manifest is not generated (which is the case in this bug) then it won't because there's no dependency on the content of the file on disk. That is what an input dependency is supposed to provide but declaring one doesn't seem to accomplish that as demonstrated by the .ninja files earlier in this thread.

I don't have time to continue investigating this issue as my sheriff rotation is long over. I will be reassigning the issue to Scott as an OWNER for //build/config/win/manifest.gni.

Reilly Grant Software Engineer rei...@google.com Google Chrome

[Dirk Pranke]

Looking at this finally after 7+ months ... As far as I can tell, it looks like Reilly's diagnosis was basically correct. 

If you modify the source_set() to have the manifests as inputs, they show up in the ninja file as order-only dependencies, and that's not the right way to treat them (they need to be regular dependencies so that the ninja action executes when the manifests change.

I played with a couple of different attempts at trying to hack something together that worked (including changing the source_set to an action(), but I couldn't get anything to work and the hacks wouldn't have made sense anyways. So, I agree with what Scott wrote, earlier: I think this needs a change in GN, something like an `ld_inputs` that could get propagated up into the link. Does anyone think such an extension would be a bad idea? I'm not sure what the priority on this is, but it seems like we could at least implement it without any compatibility risks.

(I seem to recall other situations where it would've been helpful to have that functionality, too).

-- Dirk

On Fri, Jan 14, 2022 at 5:53 PM Dirk Pranke <dpr...@google.com> wrote:

Given that Scott's no longer actively working on the project (or a Googler), probably best not to reassign to him. Bruce or myself might be a better bet.

Side note: Scott, should we just remove you from all of the OWNERS files in Chrome? Looks like there's ~9 directories where you're still listed.

-- Dirk

--
To unsubscribe from this group and stop receiving emails from it, send an email to gn-dev+un...@chromium.org.

[Dirk Pranke]

I've also just filed https://bugs.chromium.org/p/gn/issues/detail?id=304 about this.

-- Dirk