Skip to content

Introduce a WinRT utils library and "checked resources"#3350

Merged
DHowett-MSFT merged 4 commits intomasterfrom
dev/duhowett/utils
Nov 1, 2019
Merged

Introduce a WinRT utils library and "checked resources"#3350
DHowett-MSFT merged 4 commits intomasterfrom
dev/duhowett/utils

Conversation

@DHowett-MSFT
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT commented Oct 28, 2019

This commit introduces a C++/WinRT utility library and moves
ScopedResourceLoader into it. I decided to get uppity and introduce
something I like to call "checked resources." The idea is that every
resource reference from a library is knowable at compile time, and we
should be able to statically ensure that all resources exist.

PR Checklist

Details

This is a system that lets us immediately failfast (on launch) when a
library makes a static reference to a resource that doesn't exist at
runtime.

It exposes two new (preprocessor) APIs:

  • RS_(wchar_t): loads a localizable string resource by name.
  • USES_RESOURCE(wchar_t): marks a resource key as used, but is intended
    for loading images or passing static resource keys as parameters to
    functions that will look them up later.

Resource checking relies on diligent use of USES_RESOURCE() and RS_()
(which uses USES_RESOURCE), but can make sure we don't ship something
that'll blow up at runtime.

It works like this:

IN DEBUG MODE

  • All resource names referenced through USES_RESOURCE() are emitted
    alongside their referencing filenames and line numbers into a static
    section of the binary.
    That section is named .util$res$m.

  • We emit two sentinel values into two different sections, .util$res$a
    and .util$res$z.

  • The linker sorts all sections alphabetically before crushing them
    together into the final binary.

  • When we first construct a library's scoped resource loader, we
    iterate over every resource reference between $a and $z and check
    residency.

IN RELEASE MODE

  • All checked resource code is compiled out.

Fixes #2146.

Macros are the only way to do something this cool, incidentally.

Validation Steps Performed

Made references to a bunch of bad resources, tried to break it a lot.

It looks like this when it fails:

App.cpp

36  static const std::array<std::wstring_view, 2> settingsLoadErrorsLabels {
37      USES_RESOURCE(L"NoProfilesText"),
38      USES_RESOURCE(L"AllProfilesHiddenText_HA_JUST_KIDDING")
39  };
WinRTUtils\LibraryResources.cpp(68)\TerminalApp.dll:
    FailFast(1) tid(1034) 8000FFFF Catastrophic failure
    Msg:[Resource AllProfilesHiddenText_HA_JUST_KIDDING not found in
      scope TerminalApp/Resources (App.cpp:38)] [EnsureAllResourcesArePresent]

@DHowett-MSFT DHowett-MSFT requested review from miniksa and zadjii-msft and removed request for miniksa October 28, 2019 00:28
@DHowett-MSFT
Copy link
Contributor Author

Also (!!!) USES_RESOURCE works in a static context like settingsLoadErrorLabels, which was an awesome surprise.

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two questions:

A: Is adding another project going to just absolutely kill our builds again?

B: Hypothetical scenario: we add another lib helper to the AppLib project, and that new lib also needs to do resource loading. Now our dependencies look like this:

  • TerminalApp (dll)
    • TerminalAppLib (lib)
      • WinRTUtils (lib)
      • SomeNewHelper (lib)
        • WinRTUtils (lib)

Does this still work in that scenario?

I don't have any blocking issues (other than Michael's sln thing), but I'd love to hear answers before I signoff.

@zadjii-msft zadjii-msft added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Oct 28, 2019
Copy link
Contributor Author

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A single library with multiple statically linked libraries containing their own resources should be considered ill-formed, and this will be detected at link time (when there's duplicate definitions of all the functions in LibraryResources). If it's not detected at link time, it'll be detected at runtime when all of a single library's resources failfast on launch because one of the scopes won out.

@DHowett-MSFT
Copy link
Contributor Author

I think our builds will be fine :)

This commit introduces a C++/WinRT utility library and moves
ScopedResourceLoader into it. I decided to get uppity and introduce
something I like to call "checked resources." The idea is that every
resource reference from a library is knowable at compile time, and we
should be able to statically ensure that all resources exist.

This is a system that lets us immediately failfast (on launch) when a
library makes a static reference to a resource that doesn't exist at
runtime.

It exposes two new (preprocessor) APIs:
* RS_(wchar_t): loads a localizable string resource by name.
* USES_RESOURCE(wchar_t): marks a resource key as used, but is intended
  for loading images or passing static resource keys as parameters to
  functions that will look them up later.

Resource checking relies on diligent use of USES_RESOURCE() and RS_()
(which uses USES_RESOURCE), but can make sure we don't ship something
that'll blow up at runtime.

It works like this:
** IN DEBUG MODE **
- All resource names referenced through USES_RESOURCE() are emitted
  alongside their referencing filenames and line numbers into a static
  section of the binary.
  That section is named .util$res$m.

- We emit two sentinel values into two different sections, .util$res$a
  and .util$res$z.

- The linker sorts all sections alphabetically before crushing them
  together into the final binary.

- When we first construct a library's scoped resource loader, we
  iterate over every resource reference between $a and $z and check
  residency.

** IN RELEASE MODE **
- All checked resource code is compiled out.

Fixes #2146.
@DHowett-MSFT
Copy link
Contributor Author

If you attempt to use a dynamic value in RS_ or USES_RESOURCE, you get this error:

error C3493: 'x' cannot be implicitly captured because no default capture
  mode has been specified

I think it's the best we can do for this diagnostic.

@DHowett-MSFT
Copy link
Contributor Author

The CI is failing because every project referenced by UnitTests_TerminalApp is being built twice.

@zadjii-msft
Copy link
Member

I think our builds will be fine :)

lol

@DHowett-MSFT
Copy link
Contributor Author

rofl. i know, right? we have a fix that i'm pulling into a separate PR.

<ClCompile>
<PrecompiledHeaderFile>pch.h</PrecompiledHeaderFile>
<AdditionalIncludeDirectories>..;$(OpenConsoleDir)\dep\jsoncpp\json;%(AdditionalIncludeDirectories);</AdditionalIncludeDirectories>
<AdditionalIncludeDirectories>..;$(OpenConsoleDir)\src\cascadia\WinRTUtils\inc;$(OpenConsoleDir)\dep\jsoncpp\json;%(AdditionalIncludeDirectories);</AdditionalIncludeDirectories>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this particular include project be a part of cppwinrt.build.pre.props common?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could go either way on this. I like the idea of having all C++/winrt projects able to access this ;P

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decided to put it into cppwinrt pre props (@zadjii-msft may have a feeling about this)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a feeling, the feeling is 😀

Copy link
Contributor Author

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(i think i wrote comments but i have to write another comment to comment on these comments?)

@DHowett-MSFT
Copy link
Contributor Author

Apart from the build working, what does this still need to land? Do we have consensus that RS_() is "okay"?

@DHowett-MSFT
Copy link
Contributor Author

fwiw it stands for "resource string" :P

@DHowett-MSFT DHowett-MSFT merged commit 1550533 into master Nov 1, 2019
@DHowett-MSFT DHowett-MSFT deleted the dev/duhowett/utils branch November 1, 2019 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Produce a winrt-specific types library to hold some of our WinRT helpers without cluttering conhost

4 participants