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

chore: Update to TAEF 10.88.240110002 #16595

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Conversation

DHowett
Copy link
Member

@DHowett DHowett commented Jan 23, 2024

No description provided.

@DHowett
Copy link
Member Author

DHowett commented Feb 12, 2024

I'm running this PR through the PGO build as well -- i will merge when it is done. @lhecker @zadjii-msft I need a re-review on the last push

@DHowett
Copy link
Member Author

DHowett commented Feb 12, 2024

This test failure on ARM64 is durable (it keeps failing) and nonsensical.

const auto& colorTable = _renderSettings.GetColorTable();
const auto red = RGB(255, 0, 0);
const auto faintRed = RGB(127, 0, 0);
const auto green = RGB(0, 255, 0);
TextAttribute attr(red, green);
// verify that calculated foreground/background are the same as the direct
// values when reverse video is not set
VERIFY_IS_FALSE(attr.IsReverseVideo());
VERIFY_ARE_EQUAL(red, attr.GetForeground().GetColor(colorTable, _defaultFgIndex));
VERIFY_ARE_EQUAL(green, attr.GetBackground().GetColor(colorTable, _defaultBgIndex));

Line 127 is failing, because GetColor is returning #0C0C0C instead of #00FF00. We just constructed the attribute with green as the background index above.

I need to look at this on an actual ARM64 device...

@DHowett
Copy link
Member Author

DHowett commented Feb 12, 2024

Also it only fails in release.

Figures.

@DHowett
Copy link
Member Author

DHowett commented Feb 12, 2024

I am starting to suspect an ARM64 code generation issue.

image

javaw_2xkLxkOjd0

attr.GetBackground().GetColor(...) optimizes down into a direct load of index 0 from the color table.

This is after the entire attribute is const-initialized to for sure be IsRGB.

@DHowett
Copy link
Member Author

DHowett commented Feb 12, 2024

Here's a less red-yarn-and-thumbtacks image of the comparison and data flow

devenv_5CdphvOEYk

@zadjii-msft
Copy link
Member

Did we ever file an internal issue on the ARM codegen thing here?

@lhecker
Copy link
Member

lhecker commented Feb 22, 2024

@zadjii-msft
Copy link
Member

got it, so that's https://task.ms/dd/1969044 internally. For the sake of triaging the PR backlog, I'm gonna move this to a draft (since we can't merge till the compiler bug is fixed)

@zadjii-msft zadjii-msft marked this pull request as draft February 22, 2024 14:45
@zadjii-msft zadjii-msft added the Tracking-External This bug isn't resolved, but it's following an external workitem. label Feb 22, 2024
@DHowett
Copy link
Member Author

DHowett commented Feb 22, 2024

we can't merge till the compiler bug is fixed

The compiler bug reproduces in complete isolation, without TAEF. I don't think this PR caused it, it only found it.

@DHowett
Copy link
Member Author

DHowett commented Apr 29, 2024

If this arm64 build passes, I will reupdate and ingest taef 10.92

@DHowett DHowett marked this pull request as ready for review April 29, 2024 17:06
@zadjii-msft zadjii-msft added this to the Terminal v1.21 milestone Apr 29, 2024
@DHowett
Copy link
Member Author

DHowett commented Apr 29, 2024

The compiler bug may only be fixed by VS 17.11 (!)

@DHowett DHowett marked this pull request as draft April 29, 2024 17:42
@DHowett
Copy link
Member Author

DHowett commented Apr 29, 2024

Banished back to draft status

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tracking-External This bug isn't resolved, but it's following an external workitem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants