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.

Sign up for GitHub

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

Jump to bottom

CONSOLE_INFORMATION has an ODR violation contributing to test instability #3170

Closed
DHowett-MSFT opened this issue Oct 12, 2019 · 2 comments
Closed

CONSOLE_INFORMATION has an ODR violation contributing to test instability #3170

DHowett-MSFT opened this issue Oct 12, 2019 · 2 comments
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something Needs-Tag-Fix Doesn't match tag requirements Priority-1 A description (P1) Product-Conhost For issues in the Console codebase
Milestone
Engineering Impro...

Comments

@DHowett-MSFT
Copy link
Contributor

It turns out that there's an ODR violation! CONSOLE_INFORMATION changes size based on whether UNIT_TESTING is defined (VtIo got a new UNIT_TESTING-only member in #2525), and that gets rolled up into Host.unittest.lib. InteractivityBase.lib has a compile-time dependency on the layout of CONSOLE_INFORMATION, but InteractivityBase isn't built with a UNIT_TESTING variant. CI gets constructed in InteractivityBase, and as the linker can resolve the ODR violation in whatever way it sees fit it ends up choosing the shorter layout when packing in file-level statics.

This bug only exists in our tests. The fix in 83fcb6c moves the trampled global out of the way... but just fixes the symptom. :|

Originally posted by @DHowett-MSFT in #3107 (comment)
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Oct 12, 2019
@DHowett-MSFT DHowett-MSFT mentioned this issue Oct 12, 2019
Merged
5 tasks
@DHowett-MSFT DHowett-MSFT added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Conhost For issues in the Console codebase labels Oct 12, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Oct 12, 2019
@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Oct 14, 2019
@DHowett-MSFT DHowett-MSFT added this to the Console Backlog milestone Oct 14, 2019
DHowett-MSFT pushed a commit that referenced this issue Oct 15, 2019
@DHowett
Allow FontInfo{,Base,Desired} to store a font name > 32 wch (#3107)
b664761 
We now truncate the font name as it goes out to GDI APIs, in console API
servicing, and in the propsheet.

I attempted to defer truncating the font to as far up the stack as
possible, so as to make FontInfo usable for the broadest set of cases.

There were a couple questions that came up: I know that `Settings` gets
memset (memsat?) by the registry deserializer, and perhaps that's
another place for us to tackle. Right now, this pull request enables
fonts whose names are >= 32 characters _in Windows Terminal only_, but
the underpinnings are there for conhost as well. We'd need to explicitly
break at the API, or perhaps return a failure or log something to
telemetry.

* Should we log truncation at the API boundary to telemetry?
-> Later; followup filed (#3123)

* Should we fix Settings here, or later?
-> Later; followup filed (#3123)

* `TrueTypeFontList` is built out of things in winconp, the private
console header. Concern about interop structures.
-> Not used for interop, followup filed to clean it up (#3123)

* Is `unsigned int` right for codepage? For width?
-> Yes: codepage became UINT (from WORD) when we moved from Win16 to
Win32

This commit also includes a workaround for #3170. Growing
CONSOLE_INFORMATION made us lose the struct layout lottery during
release builds, and this was an expedient fix.

Closes #602.
Related to #3123.
@zadjii-msft
Copy link
Member

With the revert of #2525 in #3488, I don't think this is relevant anymore. There's no longer a debug-only member of CONSOLE_INFORMATION, so we can close this one out, yea? @DHowett

@zadjii-msft zadjii-msft added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 6, 2021
@DHowett
Copy link
Member

DHowett commented Dec 8, 2021

I believe so! Thanks.

@DHowett DHowett closed this as completed Dec 8, 2021
@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something Needs-Tag-Fix Doesn't match tag requirements Priority-1 A description (P1) Product-Conhost For issues in the Console codebase
Projects
None yet
Milestone
Engineering Improvements 2021
Development

No branches or pull requests
3 participants
@DHowett @DHowett-MSFT @zadjii-msft