Introduce chromium safe math

[miniksa]

Summary of the Pull Request

PR Checklist

• Closes Find a safer safe math #4013
• I work here.
• Existing tests should be OK. Real changes, just adding a lib to use.
• Couldn't find any existing docs about intsafe.
• Am core contributor.

Detailed Description of the Pull Request / Additional comments

• Can we remove min/max completely or rename it in the two projects where it had to be reintroduced? This is now moved into Remove global namespaced min/max from ft_host and propsheet #4152
• How many usages of the old safe math are there? 79
• If not a ton, can we migrate them here or in a follow on PR? This is now moved into Remove usages of old safe math #4153

Files with old safe math:

• TerminalControl: TSFInputControl.cpp
• TerminalCore: TerminalDispatch.cpp
• TerminalCore: TerminalSelection.cpp
• Host: directio.cpp
• RendererGdi: invalidate.cpp
• RendererGdi: math.cpp
• RendererGdi: paint.cpp
• RendererVt: paint.cpp
• TerminalAdapter: adaptDispatch.cpp
• Types: viewport.cpp
• Types: WindowUiaProviderBase.cpp

Validation Steps Performed

[miniksa]

I can't formally request @j4james and @lhecker on this review, but feel free to chime in if you like.

[WSLUser]

It's very interesting seeing Windows Terminal will have some of it's source code coming from Chromium. I'd also suggest configuring dependabot to update this dependency source from the github mirror as updates to the source occur.

Quick (likely dumb) question: Does Windows Calculator have code we can use vice Chromium? Or is it just much easier re-using what Chromium already uses?

[lhecker]

@WSLUser Open-Source FTW! 🎉

Windows Calculator doesn't use "native" numeric types to represent numbers, but rather this.
As such we can't use it for our purpose.

[WSLUser]

Thanks for pointing at that. Yeah that won't be helpful in our case. I'm sure they could extend out the functionality if a use-case was provided. If it had been something a bit more "native", using code from the same authority (MS) would of been better from a support perspective. Thanks for addressing.

[miniksa]

FYI, Chromium usage is an easy sell because of its compatible licensing and our company's existing engagement with Chromium in the form of the new Edge product. So yes: we do live in interesting times.

I've also moved the dependabot idea to #4165. It's a good idea, but I'm not going to bind it to this particular PR.

[lhecker]

I believe this library will be very helpful, especially because the different classes and methods neatly integrate with each other.
IMHO it'd be helpful if you document somewhere (in any way) what kind of changes you did to integrate this library. This would help when someone wants to update the library from the upstream (i.e. chromium). This of course only applies if you did any changes at all.
You can also remove the 3 build files if you'd like (BUILD.gn, DEPS and cgmanifest.json).

[lhecker]

This is the only line which you need to keep in mind when integrating this code IMO.
AFAICS SafeInt only throws an exception in case of a failure, whereas this library causes a hard crash.
You could modify the code to throw an exception instead - for instance a primitive subclass of std::range_error. 🙂

[miniksa]

I honestly didn't read every line of this before integrating it here. I presumed that one of the 4 classes of math here would throw on failure instead of crashing hard or that there was a template argument to override the behavior. Is that not true? If we need to modify this, we will. I was hoping to get this in, start using it, then modify it only if necessary when we discover problems.

[j4james]

I presumed that one of the 4 classes of math here would throw on failure instead of crashing hard

I hadn't looked at it in detail either, but I think you're right. Based on what they say in the README, I think the "checked" APIs crash on error, and the "strict" ones throw. Actually I misread that. The "strict" ones do compile time checking - not run time. But there does seem to be a template parameter that can control the error behaviour, so if there isn't already a version that throws, we should be able to derive one.

[lhecker]

This will not really be an issue in the beginning @miniksa @j4james. 🙂
Only 2 parts of the numerics API will call HandleFailure() and "crash" the process:

• CheckedNumeric's ValueOrDie - I suppose the naming of the function is very obvious 😄
• checked_cast<Dst, CheckHandler = CheckOnFailure, Src>() - we can just use gsl::narrow_cast instead, or simply create an alias for it in the til namespace with a replacement for the CheckHandler

Either way we should really only be careful about checked_cast at most IMO.
Also "crashing" during an erroneous checked cast is actually not even that bad of an idea in my opinion. 😶
I just wanted to mention this circumstance because I know that gsl::narrow_cast is being used, which throws an exception instead.

[miniksa]

IMHO it'd be helpful if you document somewhere (in any way) what kind of changes you did to integrate this library. This would help when someone wants to update the library from the upstream (i.e. chromium). This of course only applies if you did any changes at all.
You can also remove the 3 build files if you'd like (BUILD.gn, DEPS and cgmanifest.json).

I did zero changes to it. That's why BUILD.gn and DEPS are still there.
I introduced cgmanifest.json myself. That file is one of the ways Microsoft tracks dependencies in our own governance systems.
Then the only other special thing I did was copy the LICENSE into a subsection of our NOTICE file in the root.

Do you think I should put all that in a MAINTAINER_README.md or something?

[DHowett-MSFT]

We may actually want to namespace a bunch of this stuff into til (with using), more than what @lhecker's already suggested. It'll make us more robust if we ever do want to change things up in the future, and avoid littering chromium::numerics::whatever all over every part of the code.

[lhecker]

IMO briefly documenting how you vendored dependencies is worth the time, because the description can be quite short, but still be tremendously helpful.
For instance imagine if you note down:

• the source URL and the Git hash of the version you checked out
• just a short "yeah I didn't change anything" message

...it will allow the next person to be like: "Yup, I just need to download it from here and don't need to be careful at all since nothing was changed previously either. ™️ That 9000 line diff is all due to upstream changes which don't need any review. ™️" 😄 But in all seriousness, as you can imagine, that can be quite a stress relief.
In this particular case it might be of small benefit, but in general? I guess yeah, it's a good idea.

[miniksa]

IMO briefly documenting how you vendored dependencies is worth the time, because the description can be quite short, but still be tremendously helpful.
For instance imagine if you note down:

• the source URL and the Git hash of the version you checked out
• just a short "yeah I didn't change anything" message

...it will allow the next person to be like: "Yup, I just need to download it from here and don't need to be careful at all since nothing was changed previously either. ™️ That 9000 line diff is all due to upstream changes which don't need any review. ™️" 😄 But in all seriousness, as you can imagine, that can be quite a stress relief.
In this particular case it might be of small benefit, but in general? I guess yeah, it's a good idea.

It's done.

[miniksa]

We may actually want to namespace a bunch of this stuff into til (with using), more than what @lhecker's already suggested. It'll make us more robust if we ever do want to change things up in the future, and avoid littering chromium::numerics::whatever all over every part of the code.

Now in #4179 for someone enterprising (my bet is on @skyline75489) to pick up and do after this is merged.

[DHowett-MSFT]

min and max were kill, so fix those up and we're good to go (like the crunchwrap supreme)

[ghost]

Hello @miniksa!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.