Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mtouch/mmp] Fix tracking of whether the static registrar should run again or not. Fixes #641. (#3534) #3536

Merged
merged 1 commit into from
Feb 20, 2018

Conversation

rolfbjarne
Copy link
Member

  • [tests] Improve debug spew for the RebuildTest_WithExtensions test.

  • [mtouch/mmp] Store/load if the dynamic registrar is removed or not into the cached link results.

Store/load if the dynamic registrar is removed or not into the cached link
results, so that we generate the correct main.m even if cached linker results
are used.

  • [mtouch/mmp] The static registrar must not execute if we're loading cached results from the linker.

The static registrar must not execute if we're loading cached results from the
linker, because the static registrar needs information from the linker that's
not restored from the cache.

  • [mtouch/mmp] Share Touch code.

  • [mtouch/mmp] Make it possible to touch inexistent files (to create them).

  • [mtouch/mmp] Fix tracking of whether the static registrar should run again or not.

The recent changes to support optimizing away the dynamic registrar caused the
Xamarin.MTouch.RebuildTest_WithExtensions test to regress.

The problem

  • The linker now collects and stores information the static registrar needs.
  • This information is not restored from disk when the linker realizes that it
    can reload previously linked assemblies instead of executing again.
  • The static registrar runs again (for another reason).
  • The information the static registrar needs isn't available, and incorrect
    output follows.

So fix 1: show an error if the static registrar runs when the linker loaded
cached results.

The exact scenario the test ran into is this:

  • 1st build: everything is new and everything is built.
  • 2nd build: contents of .exe changes, the linker runs again, the static
    registrar runs again, but sees that the generated output didn't change, so
    it doesn't write the new content to disk (this is an optimization to avoid
    compiling the registrar.m file again unless needed).
  • 3rd build: only the .exe timestamp changes, the linker sees nothing changes
    in the contents of the .exe and loads the previously linked assemblies from
    disk, the static registrar sees that the .exe's timestamp is newer than
    registrar.m's timestamp and run again, but doesn't produce the right result
    because it doesn't have the information it needs.

Considered solutions

  1. Only track timestamps, not file contents. This is not ideal, since it will
    result in more work done: in particular for the case above, it would add a
    registrar.m compilation in build [appkit] Add NSImage lazy initialization #2, and linker rerun + static registrar
    rerun + registrar.m compilation + final native link in build [linker] Fix debugging of watchOS apps when link all is used. Fixes #40641 #3.
  2. Always write the output of the static registrar, even if it hasn't changed.
    This is not ideal either, since it will also result in more work done: for
    the case above, it would add a registrar.m compilation + final native link
    in build [linker] Fix debugging of watchOS apps when link all is used. Fixes #40641 #3.
  3. Always write the output of the static registrar, but track if it changed or
    not, and if it didn't, just touch registrar.o instead of recompiling it.
    This only means the final native link in build [linker] Fix debugging of watchOS apps when link all is used. Fixes #40641 #3 is added (see [README] Add a Contributing section. #5 for why
    this is worse than it sounds).
  4. Always write the output of the static registrar, but track it it changed or
    not, and if it didn't, just touch registrar.o instead of recompiling it,
    and track that too, so that the final native link in build [linker] Fix debugging of watchOS apps when link all is used. Fixes #40641 #3 isn't needed
    anymore. Unfortunately this may result in incorrect behavior, because now
    the msbuild tasks will detect that the executable has changed, and may run
    dsymutil + strip again. The executable didn't actually change, which means
    it would be the previously stripped executable, and thus we'd end up with
    an empty .dSYM because we ran dsymtil on an already stripped executable.
  5. Idea [Foundation] Add null check to NSObject.AddObserver overload #4, but write the output of the final link into a temporary directory
    instead of the .app, so that we could track whether we should update the
    executable in the .app or not. This is not optimal either, because
    executables can be big (I've seen multi-GB tvOS bitcode executables), and
    extra copies of such files should not be taken lightly.
  6. Idea [Foundation] Add null check to NSObject.AddObserver overload #4, but tell the MSBuild tasks that dsymutil/strip doesn't need to be
    rerun even if the timestamp of the executable changed. This might actually
    work, but now the solution's become quite complex.

Implemented solution

Use stamp files to detect whether a file is up-to-date or not.

In particular:

  • When we don't write to a file because the new contents are identical to the
    old contents, we now touch a .stamp file. This stamp file means "the
    accompanying file was determined to be up-to-date when the stamp was
    touched."
  • When checking whether a file is up-to-date, also check for the presence of a
    .stamp file, and if it exists, use the highest timestamp between the stamp
    file and the actual file.

Now the test scenario becomes:

  • 1st build: everything is new and everything is built.
  • 2nd build: contents of .exe changes, the linker runs again, the static
    registrar runs again, but sees that the generated output didn't change, so
    it doesn't write the new content to disk, but it creates a registrar.m.stamp
    file to indicate the point in time when registrar.m was considered up-to-
    date.
  • 3rd build: only the .exe timestamp changes, the linker sees nothing changes
    in the contents of the .exe and loads the previously linked assemblies from
    disk, the static registrar sees that the .exe's timestamp is older than
    registrar.m.stamp's timestamp and doesn't run again.

We only use the stamp file for source code (registrar.[m|h], main.[m|h],
pinvokes.[m|h]), since using it every time has too much potential for running
into other problems (for instance we should never create .stamp files inside
the .app).

Fixes these test failures:

1) Failed : Xamarin.MTouch.RebuildTest_WithExtensions("single","",False,System.String[])
  single
  Expected: <empty>
  But was:  < "/Users/builder/data/lanes/5746/4123bf7e/source/xamarin-macios/tests/mtouch/bin/Debug/tmp-test-dir/Xamarin.Tests.BundlerTool.CreateTemporaryDirectory371/testApp.app/testApp is modified, timestamp: 2/15/2018 3:04:11 PM > 2/15/2018 3:04:09 PM" >

2) Failed : Xamarin.MTouch.RebuildTest_WithExtensions("dual","armv7,arm64",False,System.String[])
  dual
  Expected: <empty>
  But was:  < "/Users/builder/data/lanes/5746/4123bf7e/source/xamarin-macios/tests/mtouch/bin/Debug/tmp-test-dir/Xamarin.Tests.BundlerTool.CreateTemporaryDirectory375/testApp.app/testApp is modified, timestamp: 2/15/2018 3:06:03 PM > 2/15/2018 3:06:00 PM" >

3) Failed : Xamarin.MTouch.RebuildTest_WithExtensions("llvm","armv7+llvm",False,System.String[])
  llvm
  Expected: <empty>
  But was:  < "/Users/builder/data/lanes/5746/4123bf7e/source/xamarin-macios/tests/mtouch/bin/Debug/tmp-test-dir/Xamarin.Tests.BundlerTool.CreateTemporaryDirectory379/testApp.app/testApp is modified, timestamp: 2/15/2018 3:07:14 PM > 2/15/2018 3:07:12 PM" >

4) Failed : Xamarin.MTouch.RebuildTest_WithExtensions("debug","",True,System.String[])
  debug
  Expected: <empty>
  But was:  < "/Users/builder/data/lanes/5746/4123bf7e/source/xamarin-macios/tests/mtouch/bin/Debug/tmp-test-dir/Xamarin.Tests.BundlerTool.CreateTemporaryDirectory383/testApp.app/testApp is modified, timestamp: 2/15/2018 3:08:16 PM > 2/15/2018 3:08:13 PM" >

5) Failed : Xamarin.MTouch.RebuildTest_WithExtensions("single-framework","",False,System.String[])
  single-framework
  Expected: <empty>
  But was:  < "/Users/builder/data/lanes/5746/4123bf7e/source/xamarin-macios/tests/mtouch/bin/Debug/tmp-test-dir/Xamarin.Tests.BundlerTool.CreateTemporaryDirectory387/testApp.app/testApp is modified, timestamp: 2/15/2018 3:09:18 PM > 2/15/2018 3:09:16 PM" >

Fixes https://github.com/xamarin/maccore/issues/641

…again or not. Fixes xamarin#641. (xamarin#3534)

* [tests] Improve debug spew for the RebuildTest_WithExtensions test.

* [mtouch/mmp] Store/load if the dynamic registrar is removed or not into the cached link results.

Store/load if the dynamic registrar is removed or not into the cached link
results, so that we generate the correct main.m even if cached linker results
are used.

* [mtouch/mmp] The static registrar must not execute if we're loading cached results from the linker.

The static registrar must not execute if we're loading cached results from the
linker, because the static registrar needs information from the linker that's
not restored from the cache.

* [mtouch/mmp] Share Touch code.

* [mtouch/mmp] Make it possible to touch inexistent files (to create them).

* [mtouch/mmp] Fix tracking of whether the static registrar should run again or not.

The recent changes to support optimizing away the dynamic registrar caused the
Xamarin.MTouch.RebuildTest_WithExtensions test to regress.

The problem
-----------

* The linker now collects and stores information the static registrar needs.
* This information is not restored from disk when the linker realizes that it
  can reload previously linked assemblies instead of executing again.
* The static registrar runs again (for another reason).
* The information the static registrar needs isn't available, and incorrect
  output follows.

So fix 1: show an error if the static registrar runs when the linker loaded
cached results.

The exact scenario the test ran into is this:

* 1st build: everything is new and everything is built.
* 2nd build: contents of .exe changes, the linker runs again, the static
  registrar runs again, but sees that the generated output didn't change, so
  it doesn't write the new content to disk (this is an optimization to avoid
  compiling the registrar.m file again unless needed).
* 3rd build: only the .exe timestamp changes, the linker sees nothing changes
  in the contents of the .exe and loads the previously linked assemblies from
  disk, the static registrar sees that the .exe's timestamp is newer than
  registrar.m's timestamp and run again, but doesn't produce the right result
  because it doesn't have the information it needs.

Considered solutions
--------------------

1. Only track timestamps, not file contents. This is not ideal, since it will
   result in more work done: in particular for the case above, it would add a
   registrar.m compilation in build #2, and linker rerun + static registrar
   rerun + registrar.m compilation + final native link in build #3.
2. Always write the output of the static registrar, even if it hasn't changed.
   This is not ideal either, since it will also result in more work done: for
   the case above, it would add a registrar.m compilation + final native link
   in build #3.
3. Always write the output of the static registrar, but track if it changed or
   not, and if it didn't, just touch registrar.o instead of recompiling it.
   This only means the final native link in build #3 is added (see #5 for why
   this is worse than it sounds).
4. Always write the output of the static registrar, but track it it changed or
   not, and if it didn't, just touch registrar.o instead of recompiling it,
   and track that too, so that the final native link in build #3 isn't needed
   anymore. Unfortunately this may result in incorrect behavior, because now
   the msbuild tasks will detect that the executable has changed, and may run
   dsymutil + strip again. The executable didn't actually change, which means
   it would be the previously stripped executable, and thus we'd end up with
   an empty .dSYM because we ran dsymtil on an already stripped executable.
5. Idea #4, but write the output of the final link into a temporary directory
   instead of the .app, so that we could track whether we should update the
   executable in the .app or not. This is not optimal either, because
   executables can be *big* (I've seen multi-GB tvOS bitcode executables), and
   extra copies of such files should not be taken lightly.
6. Idea #4, but tell the MSBuild tasks that dsymutil/strip doesn't need to be
   rerun even if the timestamp of the executable changed. This might actually
   work, but now the solution's become quite complex.

Implemented solution
--------------------

Use stamp files to detect whether a file is up-to-date or not.

In particular:

* When we don't write to a file because the new contents are identical to the
  old contents, we now touch a .stamp file. This stamp file means "the
  accompanying file was determined to be up-to-date when the stamp was
  touched."
* When checking whether a file is up-to-date, also check for the presence of a
  .stamp file, and if it exists, use the highest timestamp between the stamp
  file and the actual file.

Now the test scenario becomes:

* 1st build: everything is new and everything is built.
* 2nd build: contents of .exe changes, the linker runs again, the static
  registrar runs again, but sees that the generated output didn't change, so
  it doesn't write the new content to disk, but it creates a registrar.m.stamp
  file to indicate the point in time when registrar.m was considered up-to-
  date.
* 3rd build: only the .exe timestamp changes, the linker sees nothing changes
  in the contents of the .exe and loads the previously linked assemblies from
  disk, the static registrar sees that the .exe's timestamp is *older* than
  registrar.m.stamp's timestamp and doesn't run again.

We only use the stamp file for source code (registrar.[m|h], main.[m|h],
pinvokes.[m|h]), since using it every time has too much potential for running
into other problems (for instance we should never create .stamp files inside
the .app).

Fixes these test failures:

    1) Failed : Xamarin.MTouch.RebuildTest_WithExtensions("single","",False,System.String[])
      single
      Expected: <empty>
      But was:  < "/Users/builder/data/lanes/5746/4123bf7e/source/xamarin-macios/tests/mtouch/bin/Debug/tmp-test-dir/Xamarin.Tests.BundlerTool.CreateTemporaryDirectory371/testApp.app/testApp is modified, timestamp: 2/15/2018 3:04:11 PM > 2/15/2018 3:04:09 PM" >

    2) Failed : Xamarin.MTouch.RebuildTest_WithExtensions("dual","armv7,arm64",False,System.String[])
      dual
      Expected: <empty>
      But was:  < "/Users/builder/data/lanes/5746/4123bf7e/source/xamarin-macios/tests/mtouch/bin/Debug/tmp-test-dir/Xamarin.Tests.BundlerTool.CreateTemporaryDirectory375/testApp.app/testApp is modified, timestamp: 2/15/2018 3:06:03 PM > 2/15/2018 3:06:00 PM" >

    3) Failed : Xamarin.MTouch.RebuildTest_WithExtensions("llvm","armv7+llvm",False,System.String[])
      llvm
      Expected: <empty>
      But was:  < "/Users/builder/data/lanes/5746/4123bf7e/source/xamarin-macios/tests/mtouch/bin/Debug/tmp-test-dir/Xamarin.Tests.BundlerTool.CreateTemporaryDirectory379/testApp.app/testApp is modified, timestamp: 2/15/2018 3:07:14 PM > 2/15/2018 3:07:12 PM" >

    4) Failed : Xamarin.MTouch.RebuildTest_WithExtensions("debug","",True,System.String[])
      debug
      Expected: <empty>
      But was:  < "/Users/builder/data/lanes/5746/4123bf7e/source/xamarin-macios/tests/mtouch/bin/Debug/tmp-test-dir/Xamarin.Tests.BundlerTool.CreateTemporaryDirectory383/testApp.app/testApp is modified, timestamp: 2/15/2018 3:08:16 PM > 2/15/2018 3:08:13 PM" >

    5) Failed : Xamarin.MTouch.RebuildTest_WithExtensions("single-framework","",False,System.String[])
      single-framework
      Expected: <empty>
      But was:  < "/Users/builder/data/lanes/5746/4123bf7e/source/xamarin-macios/tests/mtouch/bin/Debug/tmp-test-dir/Xamarin.Tests.BundlerTool.CreateTemporaryDirectory387/testApp.app/testApp is modified, timestamp: 2/15/2018 3:09:18 PM > 2/15/2018 3:09:16 PM" >

Fixes xamarin/maccore#641
@monojenkins
Copy link
Collaborator

Build failure

@rolfbjarne
Copy link
Member Author

Test failures are unrelated

@rolfbjarne rolfbjarne merged commit 2a29d0e into xamarin:d15-7 Feb 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants