Skip to content

chore: Update to TAEF 10.93.240607003#16595

Merged
DHowett merged 10 commits intomainfrom
dev/duhowett/taef-10.88
Aug 16, 2024
Merged

chore: Update to TAEF 10.93.240607003#16595
DHowett merged 10 commits intomainfrom
dev/duhowett/taef-10.88

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.

terminal/src/buffer/out/ut_textbuffer/TextAttributeTests.cpp

Lines 116 to 127 in d3a18b9

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

zadjii-msft commented Feb 22, 2024

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

@lhecker
Copy link
Member

lhecker commented Feb 22, 2024

I believe it's this one: https://developercommunity.visualstudio.com/t/Bad-ARM64-codegen--maybe-during-constant/10583372

@zadjii-msft
Copy link
Member

zadjii-msft commented Feb 22, 2024

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

@DHowett DHowett changed the title chore: Update to TAEF 10.88.240110002 chore: Update to TAEF 10.93.240607003 Jun 24, 2024
@DHowett
Copy link
Member Author

DHowett commented Jun 24, 2024

It's still broken! AHHAHAHA

@DHowett
Copy link
Member Author

DHowett commented Jul 22, 2024

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Jul 22, 2024

Pull request contains merge conflicts.

@DHowett DHowett mentioned this pull request Jul 30, 2024
Merged
@DHowett
Copy link
Member Author

DHowett commented Aug 14, 2024

/azp run

@DHowett DHowett marked this pull request as ready for review August 14, 2024 21:00
@lhecker
Copy link
Member

lhecker commented Aug 16, 2024

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Aug 16, 2024

Azure Pipelines successfully started running 1 pipeline(s).

@DHowett DHowett enabled auto-merge (squash) August 16, 2024 22:16
@DHowett DHowett merged commit 4c018ef into main Aug 16, 2024
@DHowett DHowett deleted the dev/duhowett/taef-10.88 branch August 16, 2024 22:50
@j4james j4james mentioned this pull request Aug 18, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lhecker lhecker lhecker approved these changes

@zadjii-msft zadjii-msft zadjii-msft approved these changes

Assignees

No one assigned

Labels

Tracking-External This bug isn't resolved, but it's following an external workitem.

Projects

None yet

Milestone

Terminal v1.21

Development

Successfully merging this pull request may close these issues.

3 participants

@DHowett @zadjii-msft @lhecker