From eddd0047418ffc9a522d663894a2128b8d767cc0 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Wed, 10 Apr 2024 16:21:14 -0400 Subject: [PATCH 01/17] WIP: compact descriptor format json parser --- .../src/ContractDescriptorParser.cs | 107 ++++++++++++++++++ .../tests/ContractDescriptorParserTests.cs | 64 +++++++++++ .../cdacreader/tests/Directory.Build.props | 3 + .../cdacreader/tests/Directory.Build.targets | 3 + ...iagnostics.DataContractReader.Tests.csproj | 18 +++ 5 files changed, 195 insertions(+) create mode 100644 src/native/managed/cdacreader/src/ContractDescriptorParser.cs create mode 100644 src/native/managed/cdacreader/tests/ContractDescriptorParserTests.cs create mode 100644 src/native/managed/cdacreader/tests/Directory.Build.props create mode 100644 src/native/managed/cdacreader/tests/Directory.Build.targets create mode 100644 src/native/managed/cdacreader/tests/Microsoft.Diagnostics.DataContractReader.Tests.csproj diff --git a/src/native/managed/cdacreader/src/ContractDescriptorParser.cs b/src/native/managed/cdacreader/src/ContractDescriptorParser.cs new file mode 100644 index 0000000000000..58934d57f8203 --- /dev/null +++ b/src/native/managed/cdacreader/src/ContractDescriptorParser.cs @@ -0,0 +1,107 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Collections.Generic; +using System.Text.Json; +using System.Text.Json.Serialization; + +namespace Microsoft.Diagnostics.DataContractReader; +public partial class ContractDescriptorParser +{ + public const string TypeDescriptorSizeSigil = "!"; + public static CompactContractDescriptor? Parse(ReadOnlySpan json) + { + return JsonSerializer.Deserialize(json, ContractDescriptorContext.Default.CompactContractDescriptor); + } + + [JsonSerializable(typeof(CompactContractDescriptor))] + [JsonSerializable(typeof(int))] + [JsonSerializable(typeof(string))] + [JsonSerializable(typeof(Dictionary))] + [JsonSerializable(typeof(Dictionary))] + [JsonSerializable(typeof(Dictionary))] + [JsonSerializable(typeof(TypeDescriptor))] + [JsonSerializable(typeof(FieldDescriptor))] + internal sealed partial class ContractDescriptorContext : JsonSerializerContext + { + } + + // TODO: fix the key names to use lowercase + public class CompactContractDescriptor + { + public int Version { get; set; } + public string? Baseline { get; set; } + public Dictionary? Contracts { get; set; } + + public Dictionary? Types { get; set; } + + // TODO: globals + + [JsonExtensionData] + public Dictionary? Extras { get; set; } + } + + [JsonConverter(typeof(TypeDescriptorConverter))] + public class TypeDescriptor + { + public uint Size { get; set; } + public Dictionary? Fields { get; set; } + } + + // TODO: compact format needs a custom converter + public class FieldDescriptor + { + public string? Type { get; set; } + public int Offset { get; set; } + } + + internal sealed class TypeDescriptorConverter : JsonConverter + { + public override TypeDescriptor Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) + { + if (reader.TokenType != JsonTokenType.StartObject) + throw new JsonException(); + uint size = 0; + Dictionary? fields = new(); + while (reader.Read()) + { + switch (reader.TokenType) + { + case JsonTokenType.EndObject: + return new TypeDescriptor { Size = size, Fields = fields }; + case JsonTokenType.PropertyName: + if (reader.GetString() == TypeDescriptorSizeSigil) + { + reader.Read(); + size = reader.GetUInt32(); + // FIXME: handle duplicates? + } + else + { + string? fieldName = reader.GetString(); + reader.Read(); + var field = JsonSerializer.Deserialize(ref reader, ContractDescriptorContext.Default.FieldDescriptor); + // FIXME: duplicates? + if (fieldName != null && field != null) + fields.Add(fieldName, field); + else + throw new JsonException(); + } + break; + case JsonTokenType.Comment: + break; + default: + throw new JsonException(); + } + } + throw new JsonException(); + } + + public override void Write(Utf8JsonWriter writer, TypeDescriptor value, JsonSerializerOptions options) + { + throw new NotImplementedException(); + } + } + +} diff --git a/src/native/managed/cdacreader/tests/ContractDescriptorParserTests.cs b/src/native/managed/cdacreader/tests/ContractDescriptorParserTests.cs new file mode 100644 index 0000000000000..a3b8b618fe96d --- /dev/null +++ b/src/native/managed/cdacreader/tests/ContractDescriptorParserTests.cs @@ -0,0 +1,64 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Text.Unicode; +using Xunit; + +namespace Microsoft.Diagnostics.DataContractReader.UnitTests; + +public class ContractDescriptorParserTests +{ + + [Fact] + public void ParsesEmptyContract() + { + ReadOnlyMemory json = """ + { + "Version": 1, + "Baseline": "empty", + "Contracts": {}, + "Types": {}, + "Globals": {} + } + """u8.ToArray(); + ContractDescriptorParser.CompactContractDescriptor descriptor = ContractDescriptorParser.Parse(json.Span); + Assert.Equal(1, descriptor.Version); + Assert.Equal("empty", descriptor.Baseline); + Assert.Empty(descriptor.Contracts); + Assert.Empty(descriptor.Types); + Assert.NotNull(descriptor.Extras["Globals"]); + } + + [Fact] + public void ParseSizedTypes() + { + ReadOnlyMemory json = """ + { + "Version": 1, + "Baseline": "empty", + "Contracts": {}, + "Types": + { + "pointer": { "!" : 8}, + "int": { "!" : 4}, + "Point": { + "x": { "Type": "int", "Offset": 4 }, + "y": { "Type": "int", "Offset": 8 } + } + } + } + """u8.ToArray(); + ContractDescriptorParser.CompactContractDescriptor descriptor = ContractDescriptorParser.Parse(json.Span); + Assert.Equal(1, descriptor.Version); + Assert.Equal("empty", descriptor.Baseline); + Assert.Empty(descriptor.Contracts); + Assert.Equal(3, descriptor.Types.Count); + Assert.Equal(8u, descriptor.Types["pointer"].Size); + Assert.Equal(4u, descriptor.Types["int"].Size); + Assert.Equal(2, descriptor.Types["Point"].Fields.Count); + Assert.Equal(4, descriptor.Types["Point"].Fields["x"].Offset); + Assert.Equal(8, descriptor.Types["Point"].Fields["y"].Offset); + Assert.Equal(0u, descriptor.Types["Point"].Size); + } +} diff --git a/src/native/managed/cdacreader/tests/Directory.Build.props b/src/native/managed/cdacreader/tests/Directory.Build.props new file mode 100644 index 0000000000000..12f6f281a84f6 --- /dev/null +++ b/src/native/managed/cdacreader/tests/Directory.Build.props @@ -0,0 +1,3 @@ + + + diff --git a/src/native/managed/cdacreader/tests/Directory.Build.targets b/src/native/managed/cdacreader/tests/Directory.Build.targets new file mode 100644 index 0000000000000..beccb2d236e28 --- /dev/null +++ b/src/native/managed/cdacreader/tests/Directory.Build.targets @@ -0,0 +1,3 @@ + + + diff --git a/src/native/managed/cdacreader/tests/Microsoft.Diagnostics.DataContractReader.Tests.csproj b/src/native/managed/cdacreader/tests/Microsoft.Diagnostics.DataContractReader.Tests.csproj new file mode 100644 index 0000000000000..b359095c22947 --- /dev/null +++ b/src/native/managed/cdacreader/tests/Microsoft.Diagnostics.DataContractReader.Tests.csproj @@ -0,0 +1,18 @@ + + + true + true + true + $(NetCoreAppCurrent) + + + + + + + + + + + + From 10c58f5d1ef42bffced40681c206cf2b9e719507 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Thu, 11 Apr 2024 14:14:31 -0400 Subject: [PATCH 02/17] checkpoint: camelCase dict keys; compact fields --- .../src/ContractDescriptorParser.cs | 69 +++++++++++++++- .../tests/ContractDescriptorParserTests.cs | 82 +++++++++++++++---- 2 files changed, 133 insertions(+), 18 deletions(-) diff --git a/src/native/managed/cdacreader/src/ContractDescriptorParser.cs b/src/native/managed/cdacreader/src/ContractDescriptorParser.cs index 58934d57f8203..6e30b7efa664f 100644 --- a/src/native/managed/cdacreader/src/ContractDescriptorParser.cs +++ b/src/native/managed/cdacreader/src/ContractDescriptorParser.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Diagnostics.Contracts; using System.Text.Json; using System.Text.Json.Serialization; @@ -10,6 +11,7 @@ namespace Microsoft.Diagnostics.DataContractReader; public partial class ContractDescriptorParser { public const string TypeDescriptorSizeSigil = "!"; + public static CompactContractDescriptor? Parse(ReadOnlySpan json) { return JsonSerializer.Deserialize(json, ContractDescriptorContext.Default.CompactContractDescriptor); @@ -23,14 +25,18 @@ public partial class ContractDescriptorParser [JsonSerializable(typeof(Dictionary))] [JsonSerializable(typeof(TypeDescriptor))] [JsonSerializable(typeof(FieldDescriptor))] + [JsonSourceGenerationOptions(AllowTrailingCommas = true, + DictionaryKeyPolicy = JsonKnownNamingPolicy.Unspecified, // contracts, types and globals are case sensitive + PropertyNamingPolicy = JsonKnownNamingPolicy.CamelCase, + NumberHandling = JsonNumberHandling.AllowReadingFromString, + ReadCommentHandling = JsonCommentHandling.Skip)] internal sealed partial class ContractDescriptorContext : JsonSerializerContext { } - // TODO: fix the key names to use lowercase public class CompactContractDescriptor { - public int Version { get; set; } + public int? Version { get; set; } public string? Baseline { get; set; } public Dictionary? Contracts { get; set; } @@ -50,6 +56,7 @@ public class TypeDescriptor } // TODO: compact format needs a custom converter + [JsonConverter(typeof(FieldDescriptorConverter))] public class FieldDescriptor { public string? Type { get; set; } @@ -104,4 +111,62 @@ public override void Write(Utf8JsonWriter writer, TypeDescriptor value, JsonSeri } } + internal sealed class FieldDescriptorConverter : JsonConverter + { + public override FieldDescriptor Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) + { + if (reader.TokenType == JsonTokenType.Number || reader.TokenType == JsonTokenType.String) + return new FieldDescriptor { Offset = reader.GetInt32() }; + if (reader.TokenType != JsonTokenType.StartArray) + throw new JsonException(); + int eltIdx = 0; + string? type = null; + int offset = 0; + while (reader.Read()) + { + switch (reader.TokenType) + { + case JsonTokenType.EndArray: + return new FieldDescriptor { Type = type, Offset = offset }; + case JsonTokenType.Comment: + // don't incrment eltIdx + continue; + default: + break; + } + switch (eltIdx) + { + case 0: + { + // expect an offset - either a string or a number token + if (reader.TokenType == JsonTokenType.Number || reader.TokenType == JsonTokenType.String) + offset = reader.GetInt32(); + else + throw new JsonException(); + break; + } + case 1: + { + // expect a type - a string token + if (reader.TokenType == JsonTokenType.String) + type = reader.GetString(); + else + throw new JsonException(); + break; + } + default: + // too many elements + throw new JsonException(); + } + eltIdx++; + } + throw new JsonException(); + } + + public override void Write(Utf8JsonWriter writer, FieldDescriptor value, JsonSerializerOptions options) + { + throw new NotImplementedException(); + } + } + } diff --git a/src/native/managed/cdacreader/tests/ContractDescriptorParserTests.cs b/src/native/managed/cdacreader/tests/ContractDescriptorParserTests.cs index a3b8b618fe96d..997679bbffb98 100644 --- a/src/native/managed/cdacreader/tests/ContractDescriptorParserTests.cs +++ b/src/native/managed/cdacreader/tests/ContractDescriptorParserTests.cs @@ -10,24 +10,36 @@ namespace Microsoft.Diagnostics.DataContractReader.UnitTests; public class ContractDescriptorParserTests { + [Fact] public void ParsesEmptyContract() + { + ReadOnlyMemory json = "{}"u8.ToArray(); + ContractDescriptorParser.CompactContractDescriptor descriptor = ContractDescriptorParser.Parse(json.Span); + Assert.Null(descriptor.Version); + Assert.Null(descriptor.Baseline); + Assert.Null(descriptor.Contracts); + Assert.Null(descriptor.Types); + Assert.Null(descriptor.Extras); + } + [Fact] + public void ParsesTrivialContract() { ReadOnlyMemory json = """ { - "Version": 1, - "Baseline": "empty", - "Contracts": {}, - "Types": {}, - "Globals": {} + "version": 0, + "baseline": "empty", + "contracts": {}, + "types": {}, + "globals": {} } """u8.ToArray(); ContractDescriptorParser.CompactContractDescriptor descriptor = ContractDescriptorParser.Parse(json.Span); - Assert.Equal(1, descriptor.Version); + Assert.Equal(0, descriptor.Version); Assert.Equal("empty", descriptor.Baseline); Assert.Empty(descriptor.Contracts); Assert.Empty(descriptor.Types); - Assert.NotNull(descriptor.Extras["Globals"]); + Assert.NotNull(descriptor.Extras["globals"]); } [Fact] @@ -35,30 +47,68 @@ public void ParseSizedTypes() { ReadOnlyMemory json = """ { - "Version": 1, - "Baseline": "empty", - "Contracts": {}, - "Types": + "version": 0, + "baseline": "empty", + "contracts": {}, + "types": { "pointer": { "!" : 8}, "int": { "!" : 4}, "Point": { - "x": { "Type": "int", "Offset": 4 }, - "y": { "Type": "int", "Offset": 8 } + "x": [ 4, "int"], + "y": 8, + "!": 12 + }, + "Point3D": { // no size + "r": [ 0, "double"], + "phi": 8, + "rho": 16 } } } """u8.ToArray(); ContractDescriptorParser.CompactContractDescriptor descriptor = ContractDescriptorParser.Parse(json.Span); - Assert.Equal(1, descriptor.Version); + Assert.Equal(0, descriptor.Version); Assert.Equal("empty", descriptor.Baseline); Assert.Empty(descriptor.Contracts); - Assert.Equal(3, descriptor.Types.Count); + Assert.Equal(4, descriptor.Types.Count); Assert.Equal(8u, descriptor.Types["pointer"].Size); Assert.Equal(4u, descriptor.Types["int"].Size); Assert.Equal(2, descriptor.Types["Point"].Fields.Count); Assert.Equal(4, descriptor.Types["Point"].Fields["x"].Offset); Assert.Equal(8, descriptor.Types["Point"].Fields["y"].Offset); - Assert.Equal(0u, descriptor.Types["Point"].Size); + Assert.Equal("int", descriptor.Types["Point"].Fields["x"].Type); + Assert.Null(descriptor.Types["Point"].Fields["y"].Type); + Assert.Equal(12u, descriptor.Types["Point"].Size); + Assert.Equal(3, descriptor.Types["Point3D"].Fields.Count); + Assert.Equal(0, descriptor.Types["Point3D"].Fields["r"].Offset); + Assert.Equal(8, descriptor.Types["Point3D"].Fields["phi"].Offset); + Assert.Equal(16, descriptor.Types["Point3D"].Fields["rho"].Offset); + Assert.Equal("double", descriptor.Types["Point3D"].Fields["r"].Type); + Assert.Null(descriptor.Types["Point3D"].Fields["phi"].Type); + Assert.Equal(0u, descriptor.Types["Point3D"].Size); + } + + [Fact] + public void ParseContractsCaseSensitive() + { + ReadOnlyMemory json = """ + { + "version": 0, + "baseline": "empty", + "contracts": { + "foo": 1, + "Foo": 2 + }, + "types": {}, + "globals": {} + } + """u8.ToArray(); + ContractDescriptorParser.CompactContractDescriptor descriptor = ContractDescriptorParser.Parse(json.Span); + Assert.Equal(0, descriptor.Version); + Assert.Equal("empty", descriptor.Baseline); + Assert.Equal(2, descriptor.Contracts.Count); + Assert.Equal(1, descriptor.Contracts["foo"]); + Assert.Equal(2, descriptor.Contracts["Foo"]); } } From 6e583da7e8dbda8076d5a990c0a1e414fc9d7fe6 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Thu, 11 Apr 2024 16:54:22 -0400 Subject: [PATCH 03/17] more parsing --- .../src/ContractDescriptorParser.cs | 242 ++++++++++++++---- .../tests/ContractDescriptorParserTests.cs | 115 ++++++++- 2 files changed, 302 insertions(+), 55 deletions(-) diff --git a/src/native/managed/cdacreader/src/ContractDescriptorParser.cs b/src/native/managed/cdacreader/src/ContractDescriptorParser.cs index 6e30b7efa664f..4d26b6dc2965e 100644 --- a/src/native/managed/cdacreader/src/ContractDescriptorParser.cs +++ b/src/native/managed/cdacreader/src/ContractDescriptorParser.cs @@ -12,19 +12,27 @@ public partial class ContractDescriptorParser { public const string TypeDescriptorSizeSigil = "!"; - public static CompactContractDescriptor? Parse(ReadOnlySpan json) + /// + /// Parses the "compact" representation of a contract descriptor. + /// + /// + /// See data_descriptor.md for the format. + /// + public static ContractDescriptor? ParseCompact(ReadOnlySpan json) { - return JsonSerializer.Deserialize(json, ContractDescriptorContext.Default.CompactContractDescriptor); + return JsonSerializer.Deserialize(json, ContractDescriptorContext.Default.ContractDescriptor); } - [JsonSerializable(typeof(CompactContractDescriptor))] + [JsonSerializable(typeof(ContractDescriptor))] [JsonSerializable(typeof(int))] [JsonSerializable(typeof(string))] [JsonSerializable(typeof(Dictionary))] [JsonSerializable(typeof(Dictionary))] [JsonSerializable(typeof(Dictionary))] + [JsonSerializable(typeof(Dictionary))] [JsonSerializable(typeof(TypeDescriptor))] [JsonSerializable(typeof(FieldDescriptor))] + [JsonSerializable(typeof(GlobalDescriptor))] [JsonSourceGenerationOptions(AllowTrailingCommas = true, DictionaryKeyPolicy = JsonKnownNamingPolicy.Unspecified, // contracts, types and globals are case sensitive PropertyNamingPolicy = JsonKnownNamingPolicy.CamelCase, @@ -34,7 +42,7 @@ internal sealed partial class ContractDescriptorContext : JsonSerializerContext { } - public class CompactContractDescriptor + public class ContractDescriptor { public int? Version { get; set; } public string? Baseline { get; set; } @@ -42,7 +50,7 @@ public class CompactContractDescriptor public Dictionary? Types { get; set; } - // TODO: globals + public Dictionary? Globals { get; set; } [JsonExtensionData] public Dictionary? Extras { get; set; } @@ -51,11 +59,10 @@ public class CompactContractDescriptor [JsonConverter(typeof(TypeDescriptorConverter))] public class TypeDescriptor { - public uint Size { get; set; } + public uint? Size { get; set; } public Dictionary? Fields { get; set; } } - // TODO: compact format needs a custom converter [JsonConverter(typeof(FieldDescriptorConverter))] public class FieldDescriptor { @@ -63,13 +70,24 @@ public class FieldDescriptor public int Offset { get; set; } } + [JsonConverter(typeof(GlobalDescriptorConverter))] + public class GlobalDescriptor + { + public string? Type { get; set; } + public ulong Value { get; set; } + public bool Indirect { get; set; } + } + internal sealed class TypeDescriptorConverter : JsonConverter { + // Almost a normal dictionary converter except: + // 1. looks for a special key "!" to set the Size property + // 2. field names are property names, but treated case-sensitively public override TypeDescriptor Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) { if (reader.TokenType != JsonTokenType.StartObject) throw new JsonException(); - uint size = 0; + uint? size = null; Dictionary? fields = new(); while (reader.Read()) { @@ -97,6 +115,7 @@ public override TypeDescriptor Read(ref Utf8JsonReader reader, Type typeToConver } break; case JsonTokenType.Comment: + // unexpected - we specified to skip comments. but let's ignore anyway break; default: throw new JsonException(); @@ -113,60 +132,187 @@ public override void Write(Utf8JsonWriter writer, TypeDescriptor value, JsonSeri internal sealed class FieldDescriptorConverter : JsonConverter { + // Compact Field descriptors are either one or two element arrays + // 1. [number] - no type, offset is given as the number + // 2. [number, string] - has a type, offset is given as the number public override FieldDescriptor Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) { - if (reader.TokenType == JsonTokenType.Number || reader.TokenType == JsonTokenType.String) - return new FieldDescriptor { Offset = reader.GetInt32() }; + if (GetInt32FromToken(ref reader, out int offset)) + return new FieldDescriptor { Offset = offset }; if (reader.TokenType != JsonTokenType.StartArray) throw new JsonException(); - int eltIdx = 0; - string? type = null; - int offset = 0; - while (reader.Read()) + reader.Read(); + // two cases: + // [number] + // ^ we're here + // or + // [number, string] + // ^ we're here + if (!GetInt32FromToken(ref reader, out offset)) + throw new JsonException(); + reader.Read(); // end of array or string + if (reader.TokenType == JsonTokenType.EndArray) + return new FieldDescriptor { Offset = offset }; + if (reader.TokenType != JsonTokenType.String) + throw new JsonException(); + string? type = reader.GetString(); + reader.Read(); // end of array + if (reader.TokenType != JsonTokenType.EndArray) + throw new JsonException(); + return new FieldDescriptor { Type = type, Offset = offset }; + } + + public override void Write(Utf8JsonWriter writer, FieldDescriptor value, JsonSerializerOptions options) + { + throw new NotImplementedException(); + } + } + + internal sealed class GlobalDescriptorConverter : JsonConverter + { + public override GlobalDescriptor Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) + { + // four cases: + // 1. number - no type, direct value, given value + // 2. [number] - no type, indirect value, given aux data ptr + // 3. [number, string] - type, direct value, given value + // 4. [[number], string] - type, indirect value, given aux data ptr + + // Case 1: number + if (GetUInt64FromToken(ref reader, out ulong valueCase1)) + return new GlobalDescriptor { Value = valueCase1 }; + if (reader.TokenType != JsonTokenType.StartArray) + throw new JsonException(); + reader.Read(); + // we're in case 2 or 3 or 4 + // case 2: [number] + // ^ we're here + // case 3: [number, string] + // ^ we're here + // case 4: [[number], string] + // ^ we're here + if (reader.TokenType == JsonTokenType.StartArray) { - switch (reader.TokenType) - { - case JsonTokenType.EndArray: - return new FieldDescriptor { Type = type, Offset = offset }; - case JsonTokenType.Comment: - // don't incrment eltIdx - continue; - default: - break; - } - switch (eltIdx) + // case 4: [[number], string] + // ^ we're here + reader.Read(); // number + if (!GetUInt64FromToken(ref reader, out ulong value)) + throw new JsonException(); + reader.Read(); // end of inner array + if (reader.TokenType != JsonTokenType.EndArray) + throw new JsonException(); + reader.Read(); // string + if (reader.TokenType != JsonTokenType.String) + throw new JsonException(); + string? type = reader.GetString(); + reader.Read(); // end of outer array + if (reader.TokenType != JsonTokenType.EndArray) + throw new JsonException(); + return new GlobalDescriptor { Type = type, Value = value, Indirect = true }; + } + else + { + // case 2 or 3 + // case 2: [number] + // ^ we're here + // case 3: [number, string] + // ^ we're here + if (!GetUInt64FromToken(ref reader, out ulong valueCase2or3)) + throw new JsonException(); + reader.Read(); // end of array (case 2) or string (case 3) + if (reader.TokenType == JsonTokenType.EndArray) // it was case 2 + return new GlobalDescriptor { Value = valueCase2or3, Indirect = true }; + else if (reader.TokenType == JsonTokenType.String) // it was case 3 { - case 0: - { - // expect an offset - either a string or a number token - if (reader.TokenType == JsonTokenType.Number || reader.TokenType == JsonTokenType.String) - offset = reader.GetInt32(); - else - throw new JsonException(); - break; - } - case 1: - { - // expect a type - a string token - if (reader.TokenType == JsonTokenType.String) - type = reader.GetString(); - else - throw new JsonException(); - break; - } - default: - // too many elements + string? type = reader.GetString(); + reader.Read(); // end of array for case 3 + if (reader.TokenType != JsonTokenType.EndArray) throw new JsonException(); + return new GlobalDescriptor { Type = type, Value = valueCase2or3 }; } - eltIdx++; + else + throw new JsonException(); } - throw new JsonException(); } - public override void Write(Utf8JsonWriter writer, FieldDescriptor value, JsonSerializerOptions options) + public override void Write(Utf8JsonWriter writer, GlobalDescriptor value, JsonSerializerOptions options) { throw new NotImplementedException(); } } + // Somewhat flexible parsing of numbers, allowing json number tokens or strings as decimal or hex, possibly negatated. + private static bool GetUInt64FromToken(ref Utf8JsonReader reader, out ulong value) + { + if (reader.TokenType == JsonTokenType.Number) + { + if (reader.TryGetUInt64(out value)) + return true; + else if (reader.TryGetInt64(out long signedValue)) + { + value = (ulong)signedValue; + return true; + } + } + if (reader.TokenType == JsonTokenType.String) + { + var s = reader.GetString(); + if (s == null) + { + value = 0u; + return false; + } + if (ulong.TryParse(s, out value)) + return true; + if (long.TryParse(s, out long signedValue)) + { + value = (ulong)signedValue; + return true; + } + if ((s.StartsWith("0x") || s.StartsWith("0X")) && + ulong.TryParse(s.AsSpan(2), System.Globalization.NumberStyles.HexNumber, null, out value)) + return true; + if ((s.StartsWith("-0x") || s.StartsWith("-0X")) && + ulong.TryParse(s.AsSpan(3), System.Globalization.NumberStyles.HexNumber, null, out ulong negValue)) + { + value = ~negValue + 1; // twos complement + return true; + } + } + value = 0; + return false; + } + + // Somewhat flexible parsing of numbers, allowing json number tokens or strings as either decimal or hex, possibly negated + private static bool GetInt32FromToken(ref Utf8JsonReader reader, out int value) + { + if (reader.TokenType == JsonTokenType.Number) + { + value = reader.GetInt32(); + return true; + } + if (reader.TokenType == JsonTokenType.String) + { + var s = reader.GetString(); + if (s == null) + { + value = 0; + return false; + } + if (int.TryParse(s, out value)) + return true; + if ((s.StartsWith("0x") || s.StartsWith("0X")) && + int.TryParse(s.AsSpan(2), System.Globalization.NumberStyles.HexNumber, null, out value)) + return true; + if ((s.StartsWith("-0x") || s.StartsWith("-0X")) && + int.TryParse(s.AsSpan(3), System.Globalization.NumberStyles.HexNumber, null, out int negValue)) + { + value = -negValue; + return true; + } + } + value = 0; + return false; + } + } diff --git a/src/native/managed/cdacreader/tests/ContractDescriptorParserTests.cs b/src/native/managed/cdacreader/tests/ContractDescriptorParserTests.cs index 997679bbffb98..9feb5c68b5d53 100644 --- a/src/native/managed/cdacreader/tests/ContractDescriptorParserTests.cs +++ b/src/native/managed/cdacreader/tests/ContractDescriptorParserTests.cs @@ -15,7 +15,7 @@ public class ContractDescriptorParserTests public void ParsesEmptyContract() { ReadOnlyMemory json = "{}"u8.ToArray(); - ContractDescriptorParser.CompactContractDescriptor descriptor = ContractDescriptorParser.Parse(json.Span); + ContractDescriptorParser.ContractDescriptor descriptor = ContractDescriptorParser.ParseCompact(json.Span); Assert.Null(descriptor.Version); Assert.Null(descriptor.Baseline); Assert.Null(descriptor.Contracts); @@ -34,12 +34,13 @@ public void ParsesTrivialContract() "globals": {} } """u8.ToArray(); - ContractDescriptorParser.CompactContractDescriptor descriptor = ContractDescriptorParser.Parse(json.Span); + ContractDescriptorParser.ContractDescriptor descriptor = ContractDescriptorParser.ParseCompact(json.Span); Assert.Equal(0, descriptor.Version); Assert.Equal("empty", descriptor.Baseline); Assert.Empty(descriptor.Contracts); Assert.Empty(descriptor.Types); - Assert.NotNull(descriptor.Extras["globals"]); + Assert.Empty(descriptor.Globals); + Assert.Null(descriptor.Extras); } [Fact] @@ -64,13 +65,15 @@ public void ParseSizedTypes() "phi": 8, "rho": 16 } - } + }, + "globals": {} } """u8.ToArray(); - ContractDescriptorParser.CompactContractDescriptor descriptor = ContractDescriptorParser.Parse(json.Span); + ContractDescriptorParser.ContractDescriptor descriptor = ContractDescriptorParser.ParseCompact(json.Span); Assert.Equal(0, descriptor.Version); Assert.Equal("empty", descriptor.Baseline); Assert.Empty(descriptor.Contracts); + Assert.Empty(descriptor.Globals); Assert.Equal(4, descriptor.Types.Count); Assert.Equal(8u, descriptor.Types["pointer"].Size); Assert.Equal(4u, descriptor.Types["int"].Size); @@ -86,7 +89,7 @@ public void ParseSizedTypes() Assert.Equal(16, descriptor.Types["Point3D"].Fields["rho"].Offset); Assert.Equal("double", descriptor.Types["Point3D"].Fields["r"].Type); Assert.Null(descriptor.Types["Point3D"].Fields["phi"].Type); - Assert.Equal(0u, descriptor.Types["Point3D"].Size); + Assert.Null(descriptor.Types["Point3D"].Size); } [Fact] @@ -104,11 +107,109 @@ public void ParseContractsCaseSensitive() "globals": {} } """u8.ToArray(); - ContractDescriptorParser.CompactContractDescriptor descriptor = ContractDescriptorParser.Parse(json.Span); + ContractDescriptorParser.ContractDescriptor descriptor = ContractDescriptorParser.ParseCompact(json.Span); Assert.Equal(0, descriptor.Version); Assert.Equal("empty", descriptor.Baseline); Assert.Equal(2, descriptor.Contracts.Count); Assert.Equal(1, descriptor.Contracts["foo"]); Assert.Equal(2, descriptor.Contracts["Foo"]); } + + [Fact] + public void ParsesGlobals() + { + ReadOnlyMemory json = """ + { + "version": 0, + "baseline": "empty", + "contracts": {}, + "types": {}, + "globals": { + "globalInt": 1, + "globalPtr": [2], + "globalTypedInt": [3, "uint8"], + "globalTypedPtr": [[4], "uintptr"], + "globalHex": "0x1234", + "globalNegative": -2, + "globalStringyInt": "17", + "globalStringyNegative": "-2", + "globalNegativeHex": "-0xff", + "globalBigStringyInt": "0x123456789abcdef", + "globalStringyPtr": ["0x1234"], + "globalTypedStringyInt": ["0x1234", "int"], + "globalTypedStringyPtr": [["0x1234"], "int"] + } + } + """u8.ToArray(); + ContractDescriptorParser.ContractDescriptor descriptor = ContractDescriptorParser.ParseCompact(json.Span); + Assert.Equal(0, descriptor.Version); + Assert.Equal("empty", descriptor.Baseline); + Assert.Empty(descriptor.Contracts); + Assert.Empty(descriptor.Types); + Assert.Equal(13, descriptor.Globals.Count); + Assert.Equal((ulong)1, descriptor.Globals["globalInt"].Value); + Assert.False(descriptor.Globals["globalInt"].Indirect); + Assert.Equal((ulong)2, descriptor.Globals["globalPtr"].Value); + Assert.True(descriptor.Globals["globalPtr"].Indirect); + Assert.Equal((ulong)3, descriptor.Globals["globalTypedInt"].Value); + Assert.False(descriptor.Globals["globalTypedInt"].Indirect); + Assert.Equal("uint8", descriptor.Globals["globalTypedInt"].Type); + Assert.Equal((ulong)4, descriptor.Globals["globalTypedPtr"].Value); + Assert.True(descriptor.Globals["globalTypedPtr"].Indirect); + Assert.Equal("uintptr", descriptor.Globals["globalTypedPtr"].Type); + Assert.Equal((ulong)0x1234, descriptor.Globals["globalHex"].Value); + Assert.False(descriptor.Globals["globalHex"].Indirect); + Assert.Equal((ulong)0xfffffffffffffffe, descriptor.Globals["globalNegative"].Value); + Assert.False(descriptor.Globals["globalNegative"].Indirect); + Assert.Equal((ulong)17, descriptor.Globals["globalStringyInt"].Value); + Assert.False(descriptor.Globals["globalStringyInt"].Indirect); + Assert.Equal((ulong)0xfffffffffffffffe, descriptor.Globals["globalStringyNegative"].Value); + Assert.False(descriptor.Globals["globalStringyNegative"].Indirect); + Assert.Equal((ulong)0xffffffffffffff01, descriptor.Globals["globalNegativeHex"].Value); + Assert.False(descriptor.Globals["globalNegativeHex"].Indirect); + Assert.Equal((ulong)0x123456789abcdef, descriptor.Globals["globalBigStringyInt"].Value); + Assert.False(descriptor.Globals["globalBigStringyInt"].Indirect); + Assert.Equal((ulong)0x1234, descriptor.Globals["globalStringyPtr"].Value); + Assert.True(descriptor.Globals["globalStringyPtr"].Indirect); + Assert.Equal("int", descriptor.Globals["globalTypedStringyInt"].Type); + Assert.Equal((ulong)0x1234, descriptor.Globals["globalTypedStringyInt"].Value); + Assert.False(descriptor.Globals["globalTypedStringyInt"].Indirect); + Assert.Equal("int", descriptor.Globals["globalTypedStringyPtr"].Type); + Assert.Equal((ulong)0x1234, descriptor.Globals["globalTypedStringyPtr"].Value); + Assert.True(descriptor.Globals["globalTypedStringyPtr"].Indirect); + } + + [Fact] + void ParsesExoticOffsets() + { + ReadOnlyMemory json = """ + { + "version": 0, + "baseline": "empty", + "contracts": {}, + "types": { + "OddStruct": { + "a": -12, + "b": "0x12", + "c": "-0x12", + "d": ["0x100", "int"] + } + }, + "globals": { + } + } + """u8.ToArray(); + ContractDescriptorParser.ContractDescriptor descriptor = ContractDescriptorParser.ParseCompact(json.Span); + Assert.Equal(0, descriptor.Version); + Assert.Equal("empty", descriptor.Baseline); + Assert.Empty(descriptor.Contracts); + Assert.Empty(descriptor.Globals); + Assert.Equal(1, descriptor.Types.Count); + Assert.Equal(4, descriptor.Types["OddStruct"].Fields.Count); + Assert.Equal(-12, descriptor.Types["OddStruct"].Fields["a"].Offset); + Assert.Equal(0x12, descriptor.Types["OddStruct"].Fields["b"].Offset); + Assert.Equal(-0x12, descriptor.Types["OddStruct"].Fields["c"].Offset); + Assert.Equal(0x100, descriptor.Types["OddStruct"].Fields["d"].Offset); + Assert.Equal("int", descriptor.Types["OddStruct"].Fields["d"].Type); + } } From a454c1e6f1b73ae9f302fae7ba6a144761351cd3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aleksey=20Kliger=20=28=CE=BBgeek=29?= Date: Fri, 12 Apr 2024 15:47:06 -0400 Subject: [PATCH 04/17] Follow C# coding guidelines Co-authored-by: Aaron Robinson --- .../managed/cdacreader/src/ContractDescriptorParser.cs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/native/managed/cdacreader/src/ContractDescriptorParser.cs b/src/native/managed/cdacreader/src/ContractDescriptorParser.cs index 4d26b6dc2965e..20e38333e1a2c 100644 --- a/src/native/managed/cdacreader/src/ContractDescriptorParser.cs +++ b/src/native/managed/cdacreader/src/ContractDescriptorParser.cs @@ -108,10 +108,9 @@ public override TypeDescriptor Read(ref Utf8JsonReader reader, Type typeToConver reader.Read(); var field = JsonSerializer.Deserialize(ref reader, ContractDescriptorContext.Default.FieldDescriptor); // FIXME: duplicates? - if (fieldName != null && field != null) - fields.Add(fieldName, field); - else + if (fieldName is null || field is null) throw new JsonException(); + fields.Add(fieldName, field); } break; case JsonTokenType.Comment: @@ -221,7 +220,9 @@ public override GlobalDescriptor Read(ref Utf8JsonReader reader, Type typeToConv throw new JsonException(); reader.Read(); // end of array (case 2) or string (case 3) if (reader.TokenType == JsonTokenType.EndArray) // it was case 2 + { return new GlobalDescriptor { Value = valueCase2or3, Indirect = true }; + } else if (reader.TokenType == JsonTokenType.String) // it was case 3 { string? type = reader.GetString(); @@ -231,7 +232,9 @@ public override GlobalDescriptor Read(ref Utf8JsonReader reader, Type typeToConv return new GlobalDescriptor { Type = type, Value = valueCase2or3 }; } else + { throw new JsonException(); + } } } From ce035089d44e4cb73a8d8c6e2df817a31956cc94 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Fri, 12 Apr 2024 15:50:20 -0400 Subject: [PATCH 05/17] additional comments --- .../cdacreader/src/ContractDescriptorParser.cs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/native/managed/cdacreader/src/ContractDescriptorParser.cs b/src/native/managed/cdacreader/src/ContractDescriptorParser.cs index 20e38333e1a2c..98594507ff04c 100644 --- a/src/native/managed/cdacreader/src/ContractDescriptorParser.cs +++ b/src/native/managed/cdacreader/src/ContractDescriptorParser.cs @@ -8,16 +8,21 @@ using System.Text.Json.Serialization; namespace Microsoft.Diagnostics.DataContractReader; + +/// +/// A parser for the JSON representation of a contract descriptor. +/// +/// +/// See docs/design/datacontracts/data_descriptor.md for the format +/// public partial class ContractDescriptorParser { + // data_descriptor.md uses a distinguished property name to indicate the size of a type public const string TypeDescriptorSizeSigil = "!"; /// /// Parses the "compact" representation of a contract descriptor. /// - /// - /// See data_descriptor.md for the format. - /// public static ContractDescriptor? ParseCompact(ReadOnlySpan json) { return JsonSerializer.Deserialize(json, ContractDescriptorContext.Default.ContractDescriptor); From 28d7afdb6ea11ea2e17c463962cf51afb4bd361c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aleksey=20Kliger=20=28=CE=BBgeek=29?= Date: Fri, 12 Apr 2024 16:00:45 -0400 Subject: [PATCH 06/17] better doc comments Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com> --- src/native/managed/cdacreader/src/ContractDescriptorParser.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/native/managed/cdacreader/src/ContractDescriptorParser.cs b/src/native/managed/cdacreader/src/ContractDescriptorParser.cs index 98594507ff04c..88e163b6ebb2a 100644 --- a/src/native/managed/cdacreader/src/ContractDescriptorParser.cs +++ b/src/native/managed/cdacreader/src/ContractDescriptorParser.cs @@ -13,7 +13,7 @@ namespace Microsoft.Diagnostics.DataContractReader; /// A parser for the JSON representation of a contract descriptor. /// /// -/// See docs/design/datacontracts/data_descriptor.md for the format +/// See design doc for the format. /// public partial class ContractDescriptorParser { From 15b4ecbba58608b0b99f7d1f0a60a47100850f21 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Tue, 16 Apr 2024 11:41:16 -0400 Subject: [PATCH 07/17] suggestions from reviews; remove FieldDescriptor wrong conversion we incorrectly allowed `[number]` as a field descriptor conversion. that's not allowed. removed it. --- .../src/ContractDescriptorParser.cs | 54 +++++++++---------- .../tests/ContractDescriptorParserTests.cs | 36 ++++++------- 2 files changed, 43 insertions(+), 47 deletions(-) diff --git a/src/native/managed/cdacreader/src/ContractDescriptorParser.cs b/src/native/managed/cdacreader/src/ContractDescriptorParser.cs index 88e163b6ebb2a..2b66267c7b21e 100644 --- a/src/native/managed/cdacreader/src/ContractDescriptorParser.cs +++ b/src/native/managed/cdacreader/src/ContractDescriptorParser.cs @@ -101,16 +101,16 @@ public override TypeDescriptor Read(ref Utf8JsonReader reader, Type typeToConver case JsonTokenType.EndObject: return new TypeDescriptor { Size = size, Fields = fields }; case JsonTokenType.PropertyName: - if (reader.GetString() == TypeDescriptorSizeSigil) + string? fieldNameOrSizeSigil = reader.GetString(); + reader.Read(); // read the next value: either a number or a field descriptor + if (fieldNameOrSizeSigil == TypeDescriptorSizeSigil) { - reader.Read(); size = reader.GetUInt32(); // FIXME: handle duplicates? } else { - string? fieldName = reader.GetString(); - reader.Read(); + string? fieldName = fieldNameOrSizeSigil; var field = JsonSerializer.Deserialize(ref reader, ContractDescriptorContext.Default.FieldDescriptor); // FIXME: duplicates? if (fieldName is null || field is null) @@ -136,27 +136,21 @@ public override void Write(Utf8JsonWriter writer, TypeDescriptor value, JsonSeri internal sealed class FieldDescriptorConverter : JsonConverter { - // Compact Field descriptors are either one or two element arrays - // 1. [number] - no type, offset is given as the number - // 2. [number, string] - has a type, offset is given as the number + // Compact Field descriptors are either a number or a two element array + // 1. number - no type, offset is given as the number + // 2. [number, string] - offset is given as the number, type name is given as the string public override FieldDescriptor Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) { - if (GetInt32FromToken(ref reader, out int offset)) + if (TryGetInt32FromToken(ref reader, out int offset)) return new FieldDescriptor { Offset = offset }; if (reader.TokenType != JsonTokenType.StartArray) throw new JsonException(); reader.Read(); - // two cases: - // [number] - // ^ we're here - // or // [number, string] // ^ we're here - if (!GetInt32FromToken(ref reader, out offset)) + if (!TryGetInt32FromToken(ref reader, out offset)) throw new JsonException(); reader.Read(); // end of array or string - if (reader.TokenType == JsonTokenType.EndArray) - return new FieldDescriptor { Offset = offset }; if (reader.TokenType != JsonTokenType.String) throw new JsonException(); string? type = reader.GetString(); @@ -168,7 +162,7 @@ public override FieldDescriptor Read(ref Utf8JsonReader reader, Type typeToConve public override void Write(Utf8JsonWriter writer, FieldDescriptor value, JsonSerializerOptions options) { - throw new NotImplementedException(); + throw new JsonException(); } } @@ -183,7 +177,7 @@ public override GlobalDescriptor Read(ref Utf8JsonReader reader, Type typeToConv // 4. [[number], string] - type, indirect value, given aux data ptr // Case 1: number - if (GetUInt64FromToken(ref reader, out ulong valueCase1)) + if (TryGetUInt64FromToken(ref reader, out ulong valueCase1)) return new GlobalDescriptor { Value = valueCase1 }; if (reader.TokenType != JsonTokenType.StartArray) throw new JsonException(); @@ -200,7 +194,7 @@ public override GlobalDescriptor Read(ref Utf8JsonReader reader, Type typeToConv // case 4: [[number], string] // ^ we're here reader.Read(); // number - if (!GetUInt64FromToken(ref reader, out ulong value)) + if (!TryGetUInt64FromToken(ref reader, out ulong value)) throw new JsonException(); reader.Read(); // end of inner array if (reader.TokenType != JsonTokenType.EndArray) @@ -221,7 +215,7 @@ public override GlobalDescriptor Read(ref Utf8JsonReader reader, Type typeToConv // ^ we're here // case 3: [number, string] // ^ we're here - if (!GetUInt64FromToken(ref reader, out ulong valueCase2or3)) + if (!TryGetUInt64FromToken(ref reader, out ulong valueCase2or3)) throw new JsonException(); reader.Read(); // end of array (case 2) or string (case 3) if (reader.TokenType == JsonTokenType.EndArray) // it was case 2 @@ -245,18 +239,18 @@ public override GlobalDescriptor Read(ref Utf8JsonReader reader, Type typeToConv public override void Write(Utf8JsonWriter writer, GlobalDescriptor value, JsonSerializerOptions options) { - throw new NotImplementedException(); + throw new JsonException(); } } // Somewhat flexible parsing of numbers, allowing json number tokens or strings as decimal or hex, possibly negatated. - private static bool GetUInt64FromToken(ref Utf8JsonReader reader, out ulong value) + private static bool TryGetUInt64FromToken(ref Utf8JsonReader reader, out ulong value) { if (reader.TokenType == JsonTokenType.Number) { if (reader.TryGetUInt64(out value)) return true; - else if (reader.TryGetInt64(out long signedValue)) + if (reader.TryGetInt64(out long signedValue)) { value = (ulong)signedValue; return true; @@ -277,13 +271,13 @@ private static bool GetUInt64FromToken(ref Utf8JsonReader reader, out ulong valu value = (ulong)signedValue; return true; } - if ((s.StartsWith("0x") || s.StartsWith("0X")) && + if (s.StartsWith("0x", StringComparison.OrdinalIgnoreCase) && ulong.TryParse(s.AsSpan(2), System.Globalization.NumberStyles.HexNumber, null, out value)) return true; - if ((s.StartsWith("-0x") || s.StartsWith("-0X")) && + if (s.StartsWith("-0x", StringComparison.OrdinalIgnoreCase) && ulong.TryParse(s.AsSpan(3), System.Globalization.NumberStyles.HexNumber, null, out ulong negValue)) { - value = ~negValue + 1; // twos complement + value = ~negValue + 1; // two's complement return true; } } @@ -292,7 +286,7 @@ private static bool GetUInt64FromToken(ref Utf8JsonReader reader, out ulong valu } // Somewhat flexible parsing of numbers, allowing json number tokens or strings as either decimal or hex, possibly negated - private static bool GetInt32FromToken(ref Utf8JsonReader reader, out int value) + private static bool TryGetInt32FromToken(ref Utf8JsonReader reader, out int value) { if (reader.TokenType == JsonTokenType.Number) { @@ -308,11 +302,15 @@ private static bool GetInt32FromToken(ref Utf8JsonReader reader, out int value) return false; } if (int.TryParse(s, out value)) + { return true; - if ((s.StartsWith("0x") || s.StartsWith("0X")) && + } + if (s.StartsWith("0x", StringComparison.OrdinalIgnoreCase) && int.TryParse(s.AsSpan(2), System.Globalization.NumberStyles.HexNumber, null, out value)) + { return true; - if ((s.StartsWith("-0x") || s.StartsWith("-0X")) && + } + if (s.StartsWith("-0x", StringComparison.OrdinalIgnoreCase) && int.TryParse(s.AsSpan(3), System.Globalization.NumberStyles.HexNumber, null, out int negValue)) { value = -negValue; diff --git a/src/native/managed/cdacreader/tests/ContractDescriptorParserTests.cs b/src/native/managed/cdacreader/tests/ContractDescriptorParserTests.cs index 9feb5c68b5d53..eca740870727b 100644 --- a/src/native/managed/cdacreader/tests/ContractDescriptorParserTests.cs +++ b/src/native/managed/cdacreader/tests/ContractDescriptorParserTests.cs @@ -9,13 +9,11 @@ namespace Microsoft.Diagnostics.DataContractReader.UnitTests; public class ContractDescriptorParserTests { - - [Fact] public void ParsesEmptyContract() { - ReadOnlyMemory json = "{}"u8.ToArray(); - ContractDescriptorParser.ContractDescriptor descriptor = ContractDescriptorParser.ParseCompact(json.Span); + ReadOnlySpan json = "{}"u8; + ContractDescriptorParser.ContractDescriptor descriptor = ContractDescriptorParser.ParseCompact(json); Assert.Null(descriptor.Version); Assert.Null(descriptor.Baseline); Assert.Null(descriptor.Contracts); @@ -25,7 +23,7 @@ public void ParsesEmptyContract() [Fact] public void ParsesTrivialContract() { - ReadOnlyMemory json = """ + ReadOnlySpan json = """ { "version": 0, "baseline": "empty", @@ -33,8 +31,8 @@ public void ParsesTrivialContract() "types": {}, "globals": {} } - """u8.ToArray(); - ContractDescriptorParser.ContractDescriptor descriptor = ContractDescriptorParser.ParseCompact(json.Span); + """u8; + ContractDescriptorParser.ContractDescriptor descriptor = ContractDescriptorParser.ParseCompact(json); Assert.Equal(0, descriptor.Version); Assert.Equal("empty", descriptor.Baseline); Assert.Empty(descriptor.Contracts); @@ -46,7 +44,7 @@ public void ParsesTrivialContract() [Fact] public void ParseSizedTypes() { - ReadOnlyMemory json = """ + ReadOnlySpan json = """ { "version": 0, "baseline": "empty", @@ -68,8 +66,8 @@ public void ParseSizedTypes() }, "globals": {} } - """u8.ToArray(); - ContractDescriptorParser.ContractDescriptor descriptor = ContractDescriptorParser.ParseCompact(json.Span); + """u8; + ContractDescriptorParser.ContractDescriptor descriptor = ContractDescriptorParser.ParseCompact(json); Assert.Equal(0, descriptor.Version); Assert.Equal("empty", descriptor.Baseline); Assert.Empty(descriptor.Contracts); @@ -95,7 +93,7 @@ public void ParseSizedTypes() [Fact] public void ParseContractsCaseSensitive() { - ReadOnlyMemory json = """ + ReadOnlySpan json = """ { "version": 0, "baseline": "empty", @@ -106,8 +104,8 @@ public void ParseContractsCaseSensitive() "types": {}, "globals": {} } - """u8.ToArray(); - ContractDescriptorParser.ContractDescriptor descriptor = ContractDescriptorParser.ParseCompact(json.Span); + """u8; + ContractDescriptorParser.ContractDescriptor descriptor = ContractDescriptorParser.ParseCompact(json); Assert.Equal(0, descriptor.Version); Assert.Equal("empty", descriptor.Baseline); Assert.Equal(2, descriptor.Contracts.Count); @@ -118,7 +116,7 @@ public void ParseContractsCaseSensitive() [Fact] public void ParsesGlobals() { - ReadOnlyMemory json = """ + ReadOnlySpan json = """ { "version": 0, "baseline": "empty", @@ -140,8 +138,8 @@ public void ParsesGlobals() "globalTypedStringyPtr": [["0x1234"], "int"] } } - """u8.ToArray(); - ContractDescriptorParser.ContractDescriptor descriptor = ContractDescriptorParser.ParseCompact(json.Span); + """u8; + ContractDescriptorParser.ContractDescriptor descriptor = ContractDescriptorParser.ParseCompact(json); Assert.Equal(0, descriptor.Version); Assert.Equal("empty", descriptor.Baseline); Assert.Empty(descriptor.Contracts); @@ -182,7 +180,7 @@ public void ParsesGlobals() [Fact] void ParsesExoticOffsets() { - ReadOnlyMemory json = """ + ReadOnlySpan json = """ { "version": 0, "baseline": "empty", @@ -198,8 +196,8 @@ void ParsesExoticOffsets() "globals": { } } - """u8.ToArray(); - ContractDescriptorParser.ContractDescriptor descriptor = ContractDescriptorParser.ParseCompact(json.Span); + """u8; + ContractDescriptorParser.ContractDescriptor descriptor = ContractDescriptorParser.ParseCompact(json); Assert.Equal(0, descriptor.Version); Assert.Equal("empty", descriptor.Baseline); Assert.Empty(descriptor.Contracts); From 22867c10fd10c96de90611f002cd7d6cb896b801 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Tue, 16 Apr 2024 11:42:07 -0400 Subject: [PATCH 08/17] Make test project like the nativeoat+linker tests Dont' use libraries test infrastructure. Just normal arcade xunit support. --- src/native/managed/cdacreader/tests/Directory.Build.props | 3 --- src/native/managed/cdacreader/tests/Directory.Build.targets | 3 --- .../Microsoft.Diagnostics.DataContractReader.Tests.csproj | 5 ----- 3 files changed, 11 deletions(-) delete mode 100644 src/native/managed/cdacreader/tests/Directory.Build.props delete mode 100644 src/native/managed/cdacreader/tests/Directory.Build.targets diff --git a/src/native/managed/cdacreader/tests/Directory.Build.props b/src/native/managed/cdacreader/tests/Directory.Build.props deleted file mode 100644 index 12f6f281a84f6..0000000000000 --- a/src/native/managed/cdacreader/tests/Directory.Build.props +++ /dev/null @@ -1,3 +0,0 @@ - - - diff --git a/src/native/managed/cdacreader/tests/Directory.Build.targets b/src/native/managed/cdacreader/tests/Directory.Build.targets deleted file mode 100644 index beccb2d236e28..0000000000000 --- a/src/native/managed/cdacreader/tests/Directory.Build.targets +++ /dev/null @@ -1,3 +0,0 @@ - - - diff --git a/src/native/managed/cdacreader/tests/Microsoft.Diagnostics.DataContractReader.Tests.csproj b/src/native/managed/cdacreader/tests/Microsoft.Diagnostics.DataContractReader.Tests.csproj index b359095c22947..5ff2a619c0f63 100644 --- a/src/native/managed/cdacreader/tests/Microsoft.Diagnostics.DataContractReader.Tests.csproj +++ b/src/native/managed/cdacreader/tests/Microsoft.Diagnostics.DataContractReader.Tests.csproj @@ -1,7 +1,6 @@ true - true true $(NetCoreAppCurrent) @@ -11,8 +10,4 @@ - - - - From 28f5633e2885c52f3667a39bd10533303e6bf56e Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Tue, 16 Apr 2024 11:49:38 -0400 Subject: [PATCH 09/17] add tools.cdacreadertests subset; add to CLR_Tools_Tests test leg --- eng/Subsets.props | 4 ++++ eng/pipelines/common/evaluate-default-paths.yml | 4 ++++ eng/pipelines/runtime.yml | 3 ++- 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/eng/Subsets.props b/eng/Subsets.props index 499e9c3cb645f..c6605accd6ba8 100644 --- a/eng/Subsets.props +++ b/eng/Subsets.props @@ -369,6 +369,10 @@ Test="true" Category="clr" Condition="'$(DotNetBuildSourceOnly)' != 'true' and '$(NativeAotSupported)' == 'true'"/> + + + + diff --git a/eng/pipelines/common/evaluate-default-paths.yml b/eng/pipelines/common/evaluate-default-paths.yml index edbc1c618f606..5b557d4a03f18 100644 --- a/eng/pipelines/common/evaluate-default-paths.yml +++ b/eng/pipelines/common/evaluate-default-paths.yml @@ -163,6 +163,10 @@ jobs: - src/tools/illink/* - global.json + - subset: tools_cdacreader + include: + - src/native/managed/cdacreader/* + - subset: installer include: exclude: diff --git a/eng/pipelines/runtime.yml b/eng/pipelines/runtime.yml index 98dc5285250f4..8fe1dc57d7a04 100644 --- a/eng/pipelines/runtime.yml +++ b/eng/pipelines/runtime.yml @@ -713,7 +713,7 @@ extends: jobParameters: timeoutInMinutes: 120 nameSuffix: CLR_Tools_Tests - buildArgs: -s clr.aot+clr.iltools+libs.sfx+clr.toolstests -c $(_BuildConfig) -test + buildArgs: -s clr.aot+clr.iltools+libs.sfx+clr.toolstests+tools.cdacreadertests -c $(_BuildConfig) -test enablePublishTestResults: true testResultsFormat: 'xunit' # We want to run AOT tests when illink changes because there's share code and tests from illink which are used by AOT @@ -721,6 +721,7 @@ extends: or( eq(stageDependencies.EvaluatePaths.evaluate_paths.outputs['SetPathVars_coreclr.containsChange'], true), eq(stageDependencies.EvaluatePaths.evaluate_paths.outputs['SetPathVars_tools_illink.containsChange'], true), + eq(stageDependencies.EvaluatePaths.evaluate_paths.outputs['SetPathVars_tools_cdacreader.containsChange'], true), eq(variables['isRollingBuild'], true)) # # Build CrossDacs From aa58b4b704f221b44cd8b050d87c58e514e89638 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Tue, 16 Apr 2024 11:54:45 -0400 Subject: [PATCH 10/17] one more code review chang --- .../managed/cdacreader/src/ContractDescriptorParser.cs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/native/managed/cdacreader/src/ContractDescriptorParser.cs b/src/native/managed/cdacreader/src/ContractDescriptorParser.cs index 2b66267c7b21e..9aeb67ccb75ed 100644 --- a/src/native/managed/cdacreader/src/ContractDescriptorParser.cs +++ b/src/native/managed/cdacreader/src/ContractDescriptorParser.cs @@ -230,10 +230,7 @@ public override GlobalDescriptor Read(ref Utf8JsonReader reader, Type typeToConv throw new JsonException(); return new GlobalDescriptor { Type = type, Value = valueCase2or3 }; } - else - { - throw new JsonException(); - } + throw new JsonException(); } } From 894c7ff673800f893fcc85f5f19528478753cf58 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Tue, 16 Apr 2024 15:49:29 -0400 Subject: [PATCH 11/17] add tools.cdacreadertests for subset validation --- eng/Subsets.props | 2 ++ 1 file changed, 2 insertions(+) diff --git a/eng/Subsets.props b/eng/Subsets.props index c6605accd6ba8..9de503dd5002f 100644 --- a/eng/Subsets.props +++ b/eng/Subsets.props @@ -173,6 +173,8 @@ + + From 6f65672e8e412f9bd20b43c7b9e0187a107b8cba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aleksey=20Kliger=20=28=CE=BBgeek=29?= Date: Wed, 17 Apr 2024 10:50:09 -0400 Subject: [PATCH 12/17] Apply suggestions from code review Co-authored-by: Elinor Fung --- src/native/managed/cdacreader/src/ContractDescriptorParser.cs | 3 ++- .../Microsoft.Diagnostics.DataContractReader.Tests.csproj | 1 - 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/native/managed/cdacreader/src/ContractDescriptorParser.cs b/src/native/managed/cdacreader/src/ContractDescriptorParser.cs index 9aeb67ccb75ed..ee735e137586a 100644 --- a/src/native/managed/cdacreader/src/ContractDescriptorParser.cs +++ b/src/native/managed/cdacreader/src/ContractDescriptorParser.cs @@ -270,7 +270,9 @@ private static bool TryGetUInt64FromToken(ref Utf8JsonReader reader, out ulong v } if (s.StartsWith("0x", StringComparison.OrdinalIgnoreCase) && ulong.TryParse(s.AsSpan(2), System.Globalization.NumberStyles.HexNumber, null, out value)) + { return true; + } if (s.StartsWith("-0x", StringComparison.OrdinalIgnoreCase) && ulong.TryParse(s.AsSpan(3), System.Globalization.NumberStyles.HexNumber, null, out ulong negValue)) { @@ -317,5 +319,4 @@ private static bool TryGetInt32FromToken(ref Utf8JsonReader reader, out int valu value = 0; return false; } - } diff --git a/src/native/managed/cdacreader/tests/Microsoft.Diagnostics.DataContractReader.Tests.csproj b/src/native/managed/cdacreader/tests/Microsoft.Diagnostics.DataContractReader.Tests.csproj index 5ff2a619c0f63..d6bf5e2940f9c 100644 --- a/src/native/managed/cdacreader/tests/Microsoft.Diagnostics.DataContractReader.Tests.csproj +++ b/src/native/managed/cdacreader/tests/Microsoft.Diagnostics.DataContractReader.Tests.csproj @@ -1,7 +1,6 @@ true - true $(NetCoreAppCurrent) From b0c34da9ffd67832865deaeb2a7ca8c128641895 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Wed, 17 Apr 2024 10:53:13 -0400 Subject: [PATCH 13/17] reorder cases --- .../src/ContractDescriptorParser.cs | 47 +++++++++---------- 1 file changed, 23 insertions(+), 24 deletions(-) diff --git a/src/native/managed/cdacreader/src/ContractDescriptorParser.cs b/src/native/managed/cdacreader/src/ContractDescriptorParser.cs index ee735e137586a..bcf650908349a 100644 --- a/src/native/managed/cdacreader/src/ContractDescriptorParser.cs +++ b/src/native/managed/cdacreader/src/ContractDescriptorParser.cs @@ -150,7 +150,7 @@ public override FieldDescriptor Read(ref Utf8JsonReader reader, Type typeToConve // ^ we're here if (!TryGetInt32FromToken(ref reader, out offset)) throw new JsonException(); - reader.Read(); // end of array or string + reader.Read(); // string if (reader.TokenType != JsonTokenType.String) throw new JsonException(); string? type = reader.GetString(); @@ -189,40 +189,19 @@ public override GlobalDescriptor Read(ref Utf8JsonReader reader, Type typeToConv // ^ we're here // case 4: [[number], string] // ^ we're here - if (reader.TokenType == JsonTokenType.StartArray) - { - // case 4: [[number], string] - // ^ we're here - reader.Read(); // number - if (!TryGetUInt64FromToken(ref reader, out ulong value)) - throw new JsonException(); - reader.Read(); // end of inner array - if (reader.TokenType != JsonTokenType.EndArray) - throw new JsonException(); - reader.Read(); // string - if (reader.TokenType != JsonTokenType.String) - throw new JsonException(); - string? type = reader.GetString(); - reader.Read(); // end of outer array - if (reader.TokenType != JsonTokenType.EndArray) - throw new JsonException(); - return new GlobalDescriptor { Type = type, Value = value, Indirect = true }; - } - else + if (TryGetUInt64FromToken(ref reader, out ulong valueCase2or3)) { // case 2 or 3 // case 2: [number] // ^ we're here // case 3: [number, string] // ^ we're here - if (!TryGetUInt64FromToken(ref reader, out ulong valueCase2or3)) - throw new JsonException(); reader.Read(); // end of array (case 2) or string (case 3) if (reader.TokenType == JsonTokenType.EndArray) // it was case 2 { return new GlobalDescriptor { Value = valueCase2or3, Indirect = true }; } - else if (reader.TokenType == JsonTokenType.String) // it was case 3 + if (reader.TokenType == JsonTokenType.String) // it was case 3 { string? type = reader.GetString(); reader.Read(); // end of array for case 3 @@ -232,6 +211,26 @@ public override GlobalDescriptor Read(ref Utf8JsonReader reader, Type typeToConv } throw new JsonException(); } + if (reader.TokenType == JsonTokenType.StartArray) + { + // case 4: [[number], string] + // ^ we're here + reader.Read(); // number + if (!TryGetUInt64FromToken(ref reader, out ulong value)) + throw new JsonException(); + reader.Read(); // end of inner array + if (reader.TokenType != JsonTokenType.EndArray) + throw new JsonException(); + reader.Read(); // string + if (reader.TokenType != JsonTokenType.String) + throw new JsonException(); + string? type = reader.GetString(); + reader.Read(); // end of outer array + if (reader.TokenType != JsonTokenType.EndArray) + throw new JsonException(); + return new GlobalDescriptor { Type = type, Value = value, Indirect = true }; + } + throw new JsonException(); } public override void Write(Utf8JsonWriter writer, GlobalDescriptor value, JsonSerializerOptions options) From 01e3fdde7e370ddc6e546f27ee9569289ee6b9a2 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Wed, 17 Apr 2024 11:34:52 -0400 Subject: [PATCH 14/17] no duplicate fields/sizes in types --- .../cdacreader/src/ContractDescriptorParser.cs | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/native/managed/cdacreader/src/ContractDescriptorParser.cs b/src/native/managed/cdacreader/src/ContractDescriptorParser.cs index bcf650908349a..fbf76cd4e8d43 100644 --- a/src/native/managed/cdacreader/src/ContractDescriptorParser.cs +++ b/src/native/managed/cdacreader/src/ContractDescriptorParser.cs @@ -105,17 +105,23 @@ public override TypeDescriptor Read(ref Utf8JsonReader reader, Type typeToConver reader.Read(); // read the next value: either a number or a field descriptor if (fieldNameOrSizeSigil == TypeDescriptorSizeSigil) { - size = reader.GetUInt32(); - // FIXME: handle duplicates? + uint newSize = reader.GetUInt32(); + if (size is not null) + { + throw new JsonException($"Size specified multiple times: {size} and {newSize}"); + } + size = newSize; } else { string? fieldName = fieldNameOrSizeSigil; var field = JsonSerializer.Deserialize(ref reader, ContractDescriptorContext.Default.FieldDescriptor); - // FIXME: duplicates? if (fieldName is null || field is null) throw new JsonException(); - fields.Add(fieldName, field); + if (!fields.TryAdd(fieldName, field)) + { + throw new JsonException($"Duplicate field name: {fieldName}"); + } } break; case JsonTokenType.Comment: @@ -182,7 +188,7 @@ public override GlobalDescriptor Read(ref Utf8JsonReader reader, Type typeToConv if (reader.TokenType != JsonTokenType.StartArray) throw new JsonException(); reader.Read(); - // we're in case 2 or 3 or 4 + // we're in case 2 or 3 or 4: // case 2: [number] // ^ we're here // case 3: [number, string] From f0f8ef177862e41f7d4c9179d740b19bd66e60c5 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Thu, 18 Apr 2024 14:29:51 -0400 Subject: [PATCH 15/17] wip --- .../tests/Microsoft.Diagnostics.DataContractReader.Tests.csproj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/native/managed/cdacreader/tests/Microsoft.Diagnostics.DataContractReader.Tests.csproj b/src/native/managed/cdacreader/tests/Microsoft.Diagnostics.DataContractReader.Tests.csproj index d6bf5e2940f9c..70205e6231572 100644 --- a/src/native/managed/cdacreader/tests/Microsoft.Diagnostics.DataContractReader.Tests.csproj +++ b/src/native/managed/cdacreader/tests/Microsoft.Diagnostics.DataContractReader.Tests.csproj @@ -1,7 +1,7 @@ true - $(NetCoreAppCurrent) + $(NetCoreAppToolCurrent) From afdb85e6a511ed240aa981c709cdb61f4484b829 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Thu, 18 Apr 2024 15:45:09 -0400 Subject: [PATCH 16/17] Make all cdacreader.csproj ProjectReferences use the same AdditionalProperties Since we set Configuration and RuntimeIdentifier, if we don't pass the same AdditionalProperties in all ProjectReferences, we bad project.assets.json files --- ...iagnostics.DataContractReader.Tests.csproj | 5 ++++- src/native/managed/compile-native.proj | 20 ++----------------- src/native/managed/subproject.props | 19 ++++++++++++++++++ 3 files changed, 25 insertions(+), 19 deletions(-) create mode 100644 src/native/managed/subproject.props diff --git a/src/native/managed/cdacreader/tests/Microsoft.Diagnostics.DataContractReader.Tests.csproj b/src/native/managed/cdacreader/tests/Microsoft.Diagnostics.DataContractReader.Tests.csproj index 70205e6231572..22e8d256e01df 100644 --- a/src/native/managed/cdacreader/tests/Microsoft.Diagnostics.DataContractReader.Tests.csproj +++ b/src/native/managed/cdacreader/tests/Microsoft.Diagnostics.DataContractReader.Tests.csproj @@ -5,8 +5,11 @@ + + - + diff --git a/src/native/managed/compile-native.proj b/src/native/managed/compile-native.proj index d227466bfcebd..1867ed7d04dcb 100644 --- a/src/native/managed/compile-native.proj +++ b/src/native/managed/compile-native.proj @@ -35,24 +35,8 @@ --gcc-toolchain=$(ROOTFS_DIR)/usr - - - - - - - - - - - - - - - - @(SubprojectProps->'%(Identity)=%(Value)', ';') - - + + + + + + + + + + + + + + + + + + @(SubprojectProps->'%(Identity)=%(Value)', ';') + + From 5d91235336adccb1289fa0b240353aa5726d9b57 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Thu, 18 Apr 2024 19:01:58 -0400 Subject: [PATCH 17/17] Don't share the native compilation AdditionalProperties --- src/native/managed/compile-native.proj | 9 +++++++++ src/native/managed/subproject.props | 6 ------ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/native/managed/compile-native.proj b/src/native/managed/compile-native.proj index 1867ed7d04dcb..bcda8c5d6b57b 100644 --- a/src/native/managed/compile-native.proj +++ b/src/native/managed/compile-native.proj @@ -35,6 +35,15 @@ --gcc-toolchain=$(ROOTFS_DIR)/usr + + + + + + + + + diff --git a/src/native/managed/subproject.props b/src/native/managed/subproject.props index 48ecdc887cff7..b51d6f3428a4a 100644 --- a/src/native/managed/subproject.props +++ b/src/native/managed/subproject.props @@ -5,12 +5,6 @@ - - - - - -