[cdac] Read/store globals from contract descriptor by elinor-fung · Pull Request #101450 · dotnet/runtime

elinor-fung · 2024-04-23T20:20:10Z

Map indirect global values to corresponding values in pointer_data array from contract descriptor
Add [Try]Read<T>, [Try]ReadPointer, [Try]ReadGlobal<T> and [Try]ReadGlobalPointer to Target
Make cDAC implementation of ISOSDacInterface9.GetBreakingChangeVersion read value from globals
Create test helpers for mocking out reading from the target and providing a contract descriptor for different bitness/endianness
Add unit tests for reading global values (direct and indirect)

Contributes to #99298

dotnet-policy-service · 2024-04-23T20:20:42Z

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

elinor-fung · 2024-04-23T20:25:37Z

+ value = pointerData[value].Value;
+ }
+
+ globals[name] = (value, global.Type);


This is just using the (optional) type string for now. I expect we'll do some mapping that isn't just string-based once we also read in the types.

Co-authored-by: Aaron Robinson <arobins@microsoft.com>

lambdageek

Mostly LGTM

I'm not sure we want to do global type validation on every read. But it doesn't seem like we have enough info to know if there's a better place to do it in contracts. ok for now.
nit: I like validation methods in unit tests to include the name of the failing test (and possibly line info) of the actual test, not just the validation method, right in the message, not only in the stack trace

Co-authored-by: Aaron Robinson <arobins@microsoft.com>

- Map indirect global values to corresponding values in `pointer_data` array from contract descriptor - Add `[Try]Read<T>`, `[Try]ReadPointer`, `[Try]ReadGlobal<T>` and `[Try]ReadGlobalPointer` to `Target` - Make cDAC implementation of `ISOSDacInterface9.GetBreakingChangeVersion` read value from globals - Create test helpers for mocking out reading from the target and providing a contract descriptor for different bitness/endianness - Add unit tests for reading global values (direct and indirect)

elinor-fung · 2024-04-23T20:20:10Z

Map indirect global values to corresponding values in pointer_data array from contract descriptor
Add [Try]Read<T>, [Try]ReadPointer, [Try]ReadGlobal<T> and [Try]ReadGlobalPointer to Target
Make cDAC implementation of ISOSDacInterface9.GetBreakingChangeVersion read value from globals
Create test helpers for mocking out reading from the target and providing a contract descriptor for different bitness/endianness
Add unit tests for reading global values (direct and indirect)

Contributes to #99298

dotnet-policy-service · 2024-04-23T20:20:42Z

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

elinor-fung · 2024-04-23T20:25:37Z

+ value = pointerData[value].Value;
+ }
+
+ globals[name] = (value, global.Type);


This is just using the (optional) type string for now. I expect we'll do some mapping that isn't just string-based once we also read in the types.

Co-authored-by: Aaron Robinson <arobins@microsoft.com>

lambdageek

Mostly LGTM

I'm not sure we want to do global type validation on every read. But it doesn't seem like we have enough info to know if there's a better place to do it in contracts. ok for now.
nit: I like validation methods in unit tests to include the name of the failing test (and possibly line info) of the actual test, not just the validation method, right in the message, not only in the stack trace

Co-authored-by: Aaron Robinson <arobins@microsoft.com>

- Map indirect global values to corresponding values in `pointer_data` array from contract descriptor - Add `[Try]Read<T>`, `[Try]ReadPointer`, `[Try]ReadGlobal<T>` and `[Try]ReadGlobalPointer` to `Target` - Make cDAC implementation of `ISOSDacInterface9.GetBreakingChangeVersion` read value from globals - Create test helpers for mocking out reading from the target and providing a contract descriptor for different bitness/endianness - Add unit tests for reading global values (direct and indirect)

elinor-fung · 2024-04-23T20:20:10Z

Map indirect global values to corresponding values in pointer_data array from contract descriptor
Add [Try]Read<T>, [Try]ReadPointer, [Try]ReadGlobal<T> and [Try]ReadGlobalPointer to Target
Make cDAC implementation of ISOSDacInterface9.GetBreakingChangeVersion read value from globals
Create test helpers for mocking out reading from the target and providing a contract descriptor for different bitness/endianness
Add unit tests for reading global values (direct and indirect)

Contributes to #99298

dotnet-policy-service · 2024-04-23T20:20:42Z

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

elinor-fung · 2024-04-23T20:25:37Z

+ value = pointerData[value].Value;
+ }
+
+ globals[name] = (value, global.Type);


This is just using the (optional) type string for now. I expect we'll do some mapping that isn't just string-based once we also read in the types.

Co-authored-by: Aaron Robinson <arobins@microsoft.com>

lambdageek

Mostly LGTM

I'm not sure we want to do global type validation on every read. But it doesn't seem like we have enough info to know if there's a better place to do it in contracts. ok for now.
nit: I like validation methods in unit tests to include the name of the failing test (and possibly line info) of the actual test, not just the validation method, right in the message, not only in the stack trace

Co-authored-by: Aaron Robinson <arobins@microsoft.com>

- Map indirect global values to corresponding values in `pointer_data` array from contract descriptor - Add `[Try]Read<T>`, `[Try]ReadPointer`, `[Try]ReadGlobal<T>` and `[Try]ReadGlobalPointer` to `Target` - Make cDAC implementation of `ISOSDacInterface9.GetBreakingChangeVersion` read value from globals - Create test helpers for mocking out reading from the target and providing a contract descriptor for different bitness/endianness - Add unit tests for reading global values (direct and indirect)

elinor-fung · 2024-04-23T20:20:10Z

Map indirect global values to corresponding values in pointer_data array from contract descriptor
Add [Try]Read<T>, [Try]ReadPointer, [Try]ReadGlobal<T> and [Try]ReadGlobalPointer to Target
Make cDAC implementation of ISOSDacInterface9.GetBreakingChangeVersion read value from globals
Create test helpers for mocking out reading from the target and providing a contract descriptor for different bitness/endianness
Add unit tests for reading global values (direct and indirect)

Contributes to #99298

dotnet-policy-service · 2024-04-23T20:20:42Z

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

elinor-fung · 2024-04-23T20:25:37Z

+ value = pointerData[value].Value;
+ }
+
+ globals[name] = (value, global.Type);


This is just using the (optional) type string for now. I expect we'll do some mapping that isn't just string-based once we also read in the types.

Co-authored-by: Aaron Robinson <arobins@microsoft.com>

lambdageek

Mostly LGTM

I'm not sure we want to do global type validation on every read. But it doesn't seem like we have enough info to know if there's a better place to do it in contracts. ok for now.
nit: I like validation methods in unit tests to include the name of the failing test (and possibly line info) of the actual test, not just the validation method, right in the message, not only in the stack trace

Co-authored-by: Aaron Robinson <arobins@microsoft.com>

- Map indirect global values to corresponding values in `pointer_data` array from contract descriptor - Add `[Try]Read<T>`, `[Try]ReadPointer`, `[Try]ReadGlobal<T>` and `[Try]ReadGlobalPointer` to `Target` - Make cDAC implementation of `ISOSDacInterface9.GetBreakingChangeVersion` read value from globals - Create test helpers for mocking out reading from the target and providing a contract descriptor for different bitness/endianness - Add unit tests for reading global values (direct and indirect)

elinor-fung · 2024-04-23T20:20:10Z

Map indirect global values to corresponding values in pointer_data array from contract descriptor
Add [Try]Read<T>, [Try]ReadPointer, [Try]ReadGlobal<T> and [Try]ReadGlobalPointer to Target
Make cDAC implementation of ISOSDacInterface9.GetBreakingChangeVersion read value from globals
Create test helpers for mocking out reading from the target and providing a contract descriptor for different bitness/endianness
Add unit tests for reading global values (direct and indirect)

Contributes to #99298

dotnet-policy-service · 2024-04-23T20:20:42Z

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

elinor-fung · 2024-04-23T20:25:37Z

+ value = pointerData[value].Value;
+ }
+
+ globals[name] = (value, global.Type);


This is just using the (optional) type string for now. I expect we'll do some mapping that isn't just string-based once we also read in the types.

Co-authored-by: Aaron Robinson <arobins@microsoft.com>

lambdageek

Mostly LGTM

I'm not sure we want to do global type validation on every read. But it doesn't seem like we have enough info to know if there's a better place to do it in contracts. ok for now.
nit: I like validation methods in unit tests to include the name of the failing test (and possibly line info) of the actual test, not just the validation method, right in the message, not only in the stack trace

Co-authored-by: Aaron Robinson <arobins@microsoft.com>

- Map indirect global values to corresponding values in `pointer_data` array from contract descriptor - Add `[Try]Read<T>`, `[Try]ReadPointer`, `[Try]ReadGlobal<T>` and `[Try]ReadGlobalPointer` to `Target` - Make cDAC implementation of `ISOSDacInterface9.GetBreakingChangeVersion` read value from globals - Create test helpers for mocking out reading from the target and providing a contract descriptor for different bitness/endianness - Add unit tests for reading global values (direct and indirect)

elinor-fung · 2024-04-23T20:20:10Z

Map indirect global values to corresponding values in pointer_data array from contract descriptor
Add [Try]Read<T>, [Try]ReadPointer, [Try]ReadGlobal<T> and [Try]ReadGlobalPointer to Target
Make cDAC implementation of ISOSDacInterface9.GetBreakingChangeVersion read value from globals
Create test helpers for mocking out reading from the target and providing a contract descriptor for different bitness/endianness
Add unit tests for reading global values (direct and indirect)

Contributes to #99298

dotnet-policy-service · 2024-04-23T20:20:42Z

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

elinor-fung · 2024-04-23T20:25:37Z

+ value = pointerData[value].Value;
+ }
+
+ globals[name] = (value, global.Type);


This is just using the (optional) type string for now. I expect we'll do some mapping that isn't just string-based once we also read in the types.

Co-authored-by: Aaron Robinson <arobins@microsoft.com>

lambdageek

Mostly LGTM

I'm not sure we want to do global type validation on every read. But it doesn't seem like we have enough info to know if there's a better place to do it in contracts. ok for now.
nit: I like validation methods in unit tests to include the name of the failing test (and possibly line info) of the actual test, not just the validation method, right in the message, not only in the stack trace

Co-authored-by: Aaron Robinson <arobins@microsoft.com>

- Map indirect global values to corresponding values in `pointer_data` array from contract descriptor - Add `[Try]Read<T>`, `[Try]ReadPointer`, `[Try]ReadGlobal<T>` and `[Try]ReadGlobalPointer` to `Target` - Make cDAC implementation of `ISOSDacInterface9.GetBreakingChangeVersion` read value from globals - Create test helpers for mocking out reading from the target and providing a contract descriptor for different bitness/endianness - Add unit tests for reading global values (direct and indirect)

elinor-fung · 2024-04-23T20:20:10Z

Map indirect global values to corresponding values in pointer_data array from contract descriptor
Add [Try]Read<T>, [Try]ReadPointer, [Try]ReadGlobal<T> and [Try]ReadGlobalPointer to Target
Make cDAC implementation of ISOSDacInterface9.GetBreakingChangeVersion read value from globals
Create test helpers for mocking out reading from the target and providing a contract descriptor for different bitness/endianness
Add unit tests for reading global values (direct and indirect)

Contributes to #99298

dotnet-policy-service · 2024-04-23T20:20:42Z

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

elinor-fung · 2024-04-23T20:25:37Z

+ value = pointerData[value].Value;
+ }
+
+ globals[name] = (value, global.Type);


This is just using the (optional) type string for now. I expect we'll do some mapping that isn't just string-based once we also read in the types.

Co-authored-by: Aaron Robinson <arobins@microsoft.com>

lambdageek

Mostly LGTM

I'm not sure we want to do global type validation on every read. But it doesn't seem like we have enough info to know if there's a better place to do it in contracts. ok for now.
nit: I like validation methods in unit tests to include the name of the failing test (and possibly line info) of the actual test, not just the validation method, right in the message, not only in the stack trace

Co-authored-by: Aaron Robinson <arobins@microsoft.com>

- Map indirect global values to corresponding values in `pointer_data` array from contract descriptor - Add `[Try]Read<T>`, `[Try]ReadPointer`, `[Try]ReadGlobal<T>` and `[Try]ReadGlobalPointer` to `Target` - Make cDAC implementation of `ISOSDacInterface9.GetBreakingChangeVersion` read value from globals - Create test helpers for mocking out reading from the target and providing a contract descriptor for different bitness/endianness - Add unit tests for reading global values (direct and indirect)

elinor-fung · 2024-04-23T20:20:10Z

Map indirect global values to corresponding values in pointer_data array from contract descriptor
Add [Try]Read<T>, [Try]ReadPointer, [Try]ReadGlobal<T> and [Try]ReadGlobalPointer to Target
Make cDAC implementation of ISOSDacInterface9.GetBreakingChangeVersion read value from globals
Create test helpers for mocking out reading from the target and providing a contract descriptor for different bitness/endianness
Add unit tests for reading global values (direct and indirect)

Contributes to #99298

dotnet-policy-service · 2024-04-23T20:20:42Z

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

elinor-fung · 2024-04-23T20:25:37Z

+ value = pointerData[value].Value;
+ }
+
+ globals[name] = (value, global.Type);


This is just using the (optional) type string for now. I expect we'll do some mapping that isn't just string-based once we also read in the types.

Co-authored-by: Aaron Robinson <arobins@microsoft.com>

lambdageek

Mostly LGTM

I'm not sure we want to do global type validation on every read. But it doesn't seem like we have enough info to know if there's a better place to do it in contracts. ok for now.
nit: I like validation methods in unit tests to include the name of the failing test (and possibly line info) of the actual test, not just the validation method, right in the message, not only in the stack trace

Co-authored-by: Aaron Robinson <arobins@microsoft.com>

- Map indirect global values to corresponding values in `pointer_data` array from contract descriptor - Add `[Try]Read<T>`, `[Try]ReadPointer`, `[Try]ReadGlobal<T>` and `[Try]ReadGlobalPointer` to `Target` - Make cDAC implementation of `ISOSDacInterface9.GetBreakingChangeVersion` read value from globals - Create test helpers for mocking out reading from the target and providing a contract descriptor for different bitness/endianness - Add unit tests for reading global values (direct and indirect)

elinor-fung added 6 commits April 22, 2024 23:44

Check cdac_reader_init result
fc3dcf2

Store globals from contract descriptor
fda47d3

Read SOSBreakingChangeVersion from globals
0700338

Read global pointer
a01b167

Add tests for reading global value
8527355

Add tests for reading indirect global value
7f77200

ghost added the area-Diagnostics-coreclr label Apr 23, 2024

dotnet-policy-serviceBot assigned elinor-fung Apr 23, 2024

elinor-fung commented Apr 23, 2024
View reviewed changes

elinor-fung requested review from AaronRobinsonMSFT and lambdageek April 23, 2024 20:26

jkoritzinsky reviewed Apr 23, 2024
View reviewed changes

Comment threadsrc/native/managed/cdacreader/src/Target.cs Outdated

This was referenced Apr 24, 2024
clang crashing on osx #99150
Closed
Timeout in System.Text.Json.Tests.Utf8JsonWriterTests.WriteNumbers #101193
Closed
WasmTestOnChrome-System.Runtime.Tests.WorkItemExecution fails with exit code 3221226505 #101464
Closed

AaronRobinsonMSFT reviewed Apr 24, 2024
View reviewed changes

Comment threadsrc/coreclr/debug/daccess/cdac.cpp Outdated

build-analysisBot mentioned this pull request Apr 24, 2024
System.Text.Json.Tests timeout in Linux-Debug-x64-Mono_Interpreter_Debug-Ubuntu.2204.Amd64.Open #101508
Closed

elinor-fung added 2 commits April 24, 2024 13:16

Move calling init into CDAC::Create
c7bc9aa

Use generic with IBinaryInteger constraint
03ab101

elinor-fung force-pushed the cdac-read-globals branch from a6c9402 to 03ab101Compare April 24, 2024 20:48

AaronRobinsonMSFT reviewed Apr 24, 2024
View reviewed changes

Comment threadsrc/coreclr/debug/daccess/daccess.cpp

AaronRobinsonMSFT reviewed Apr 24, 2024
View reviewed changes

Comment threadsrc/native/managed/cdacreader/src/Target.cs Outdated

elinor-fung commented Apr 24, 2024
View reviewed changes

Comment threadsrc/coreclr/debug/daccess/daccess.cpp

elinor-fung commented Apr 24, 2024
View reviewed changes

Comment threadsrc/coreclr/debug/daccess/cdac.h Outdated

elinor-fungand others added 2 commits April 24, 2024 14:38

Apply suggestions from code review
7c773e9
Co-authored-by: Aaron Robinson <arobins@microsoft.com>

Replace [Try]Read* with generic [Try]Read for numbers
125608f

This was referenced Apr 25, 2024
CI error: System.Net.Quic.QuicException: The connection timed out from inactivity #91757
Closed
[wasm] System.Text.Json.Tests running out of memory #98578
Closed
[wasm] System.Text.Json.Tests.Utf8JsonWriterTests.WriteLargeToStream #101016
Closed

AaronRobinsonMSFT reviewed Apr 25, 2024
View reviewed changes

Comment threadsrc/native/managed/cdacreader/src/Target.cs Outdated

AaronRobinsonMSFT approved these changes Apr 25, 2024
View reviewed changes

lambdageek approved these changes Apr 25, 2024
View reviewed changes

Comment threadsrc/native/managed/cdacreader/src/Target.cs
Comment threadsrc/native/managed/cdacreader/tests/TargetTests.cs Outdated

elinor-fung commented Apr 25, 2024
View reviewed changes

Comment threadsrc/native/managed/cdacreader/src/Target.cs

elinor-fungand others added 2 commits April 25, 2024 09:44

Apply suggestions from code review
c142884
Co-authored-by: Aaron Robinson <arobins@microsoft.com>

Add caller info to asserts in test
28dc125

build-analysisBot mentioned this pull request Apr 25, 2024
[CI] Test failure from System.Net.Http.Functional.Tests.SocketsHttpHandlerTest_Cookies_Http3.GetAsync_DefaultCoookieContainer_NoCookieSent#101567
Closed

elinor-fung merged commit 2d335f3 into dotnet:mainApr 26, 2024

elinor-fung deleted the cdac-read-globals branch April 26, 2024 03:30

elinor-fung mentioned this pull request Apr 26, 2024
Portable Data Contract-based DAC #99298
Open
37 tasks

github-actionsBot locked and limited conversation to collaborators May 27, 2024