Skip to content

[tests][eventpipe] Port over Dotnet/Diagnostics to enable eventpipe tests on Android #64358

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

Merged
merged 27 commits into from
Mar 2, 2022
Merged

[tests][eventpipe] Port over Dotnet/Diagnostics to enable eventpipe tests on Android #64358

Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
ce5f9c7
[tests][eventpipe] Port over Dotnet/Diagnostics files for mobile even…
mdh1418 Jan 26, 2022
0753824
[tests][eventpipe] Add TCP/IP logic for mobile eventpipe tests
mdh1418 Feb 1, 2022
4a6eeec
[tests] Remove Microsoft.Diagnostics.NETCore.Client package reference
mdh1418 Feb 19, 2022
9e3c2fe
[tests][eventpipe] Downstream Diagnostics IpcTraceTest DiagnosticsCli…
mdh1418 Feb 19, 2022
fb602ba
[tests][eventpipe] Downstream Diagnostics roslyn analyzer IpcTraceTes…
mdh1418 Feb 19, 2022
b1c6c46
[tests][eventpipe] Enable TCPIP DiagnosticsClient in IpcTraceTest for…
mdh1418 Feb 19, 2022
781af1d
[tests][eventpipe] Aesthetic IpcTraceTest modifications
mdh1418 Feb 19, 2022
572438c
[tests][eventpipe] Disable subprocesses tests on Android
mdh1418 Feb 17, 2022
2bfb0a5
[tests][eventpipe] Update processinfo
Dec 3, 2021
88ee4f9
[tests][eventpipe] Update processinfo2
Dec 3, 2021
4bf5a8f
[tests][eventpipe] Update eventsourceerror
mdh1418 Feb 18, 2022
2e860a4
[tests][eventpipe] Update bigevent
mdh1418 Feb 18, 2022
3babfc2
[tests][eventpipe] Update buffersize
mdh1418 Feb 18, 2022
07daa4b
[tests][eventpipe] Update rundownvalidation
mdh1418 Feb 18, 2022
fe7a901
[tests][eventpipe] Update providervalidation
mdh1418 Feb 18, 2022
b2a508d
[tests][eventpipe] Update gcdump
mdh1418 Feb 18, 2022
01ea71d
[tests][JIT] Update debuginfo/tester
mdh1418 Feb 18, 2022
dc29676
[tests] Segment Microsoft.Diagnostics.NETCore.Client relevant tests f…
mdh1418 Feb 20, 2022
27f6411
Account for nonspecified RuntimeFlavor
mdh1418 Feb 22, 2022
9b58954
[tests] Moveup Default coreclr RuntimeFlavor property explicit declar…
mdh1418 Feb 22, 2022
cb2cacd
[tests] Duplicate Microsoft.Diagnostics.NETCore.Client dependent test…
mdh1418 Feb 24, 2022
1e90d7e
Fix debuginfo/tester test skip
mdh1418 Feb 24, 2022
98cf133
Temporarily enable bigevent on Linux arm and remove duplicate exclude
mdh1418 Feb 24, 2022
fe317a3
Fix unaligned UTF16 string read in collect tracing EventPipe command.
lateralusX Feb 28, 2022
8cdbf34
Revert "[tests] Duplicate Microsoft.Diagnostics.NETCore.Client depend…
mdh1418 Feb 28, 2022
7c62cd5
Revert "[tests] Segment Microsoft.Diagnostics.NETCore.Client relevant…
mdh1418 Feb 28, 2022
630d280
Revert "Fix debuginfo/tester test skip"
mdh1418 Feb 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 20 additions & 8 deletions src/native/eventpipe/ds-eventpipe-protocol.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ eventpipe_collect_tracing_command_try_parse_rundown_requested (
EP_ASSERT (buffer_len != NULL);
EP_ASSERT (rundown_requested != NULL);

return ds_ipc_message_try_parse_value (buffer, buffer_len, (uint8_t *)rundown_requested, (uint32_t)sizeof (bool));
return ds_ipc_message_try_parse_value (buffer, buffer_len, (uint8_t *)rundown_requested, 1);
}

static
Expand All @@ -160,6 +160,9 @@ eventpipe_collect_tracing_command_try_parse_config (
const uint32_t max_count_configs = 1000;
uint32_t count_configs = 0;

uint8_t *provider_name_byte_array = NULL;
uint8_t *filter_data_byte_array = NULL;

ep_char8_t *provider_name_utf8 = NULL;
ep_char8_t *filter_data_utf8 = NULL;

Expand All @@ -176,20 +179,27 @@ eventpipe_collect_tracing_command_try_parse_config (
ep_raise_error_if_nok (ds_ipc_message_try_parse_uint32_t (buffer, buffer_len, &log_level));
ep_raise_error_if_nok (log_level <= EP_EVENT_LEVEL_VERBOSE);

const ep_char16_t *provider_name = NULL;
ep_raise_error_if_nok (ds_ipc_message_try_parse_string_utf16_t (buffer, buffer_len, &provider_name));
uint32_t provider_name_byte_array_len = 0;
ep_raise_error_if_nok (ds_ipc_message_try_parse_string_utf16_t_byte_array_alloc (buffer, buffer_len, &provider_name_byte_array, &provider_name_byte_array_len));

provider_name_utf8 = ep_rt_utf16_to_utf8_string (provider_name, -1);
provider_name_utf8 = ep_rt_utf16_to_utf8_string ((const ep_char16_t *)provider_name_byte_array, -1);
ep_raise_error_if_nok (provider_name_utf8 != NULL);

ep_raise_error_if_nok (!ep_rt_utf8_string_is_null_or_empty (provider_name_utf8));

const ep_char16_t *filter_data = NULL; // This parameter is optional.
ds_ipc_message_try_parse_string_utf16_t (buffer, buffer_len, &filter_data);
ep_rt_byte_array_free (provider_name_byte_array);
provider_name_byte_array = NULL;

uint32_t filter_data_byte_array_len = 0;
ep_raise_error_if_nok (ds_ipc_message_try_parse_string_utf16_t_byte_array_alloc (buffer, buffer_len, &filter_data_byte_array, &filter_data_byte_array_len));

if (filter_data) {
filter_data_utf8 = ep_rt_utf16_to_utf8_string (filter_data, -1);
// This parameter is optional.
if (filter_data_byte_array) {
filter_data_utf8 = ep_rt_utf16_to_utf8_string ((const ep_char16_t *)filter_data_byte_array, -1);
ep_raise_error_if_nok (filter_data_utf8 != NULL);

ep_rt_byte_array_free (filter_data_byte_array);
filter_data_byte_array = NULL;
}

EventPipeProviderConfiguration provider_config;
Expand All @@ -209,7 +219,9 @@ eventpipe_collect_tracing_command_try_parse_config (

ep_on_error:
count_configs = 0;
ep_rt_byte_array_free (provider_name_byte_array);
ep_rt_utf8_string_free (provider_name_utf8);
ep_rt_byte_array_free (filter_data_byte_array);
ep_rt_utf8_string_free (filter_data_utf8);
ep_exit_error_handler ();
}
Expand Down
98 changes: 81 additions & 17 deletions src/native/eventpipe/ds-protocol.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,14 @@ ipc_message_flatten (
uint16_t payload_len,
ds_ipc_flatten_payload_func flatten_payload);

static
bool
ipc_message_try_parse_string_utf16_t_byte_array (
uint8_t **buffer,
uint32_t *buffer_len,
const uint8_t **string_byte_array,
uint32_t *string_byte_array_len);

/*
* DiagnosticsIpc
*/
Expand Down Expand Up @@ -306,6 +314,50 @@ ipc_message_flatten (
ep_exit_error_handler ();
}

static
bool
ipc_message_try_parse_string_utf16_t_byte_array (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lateralusX Is this mostly meant as a targeted fix? Feels odd we are only safekeeping this one case. I wonder if we can statically test for alignment on these reads or check dynamically if the platform supports it and perform copies when necessary. This also feels like a potential pain point for our portable builds (pretty much would for reads to always be aligned). The fix itself (copying) LGTM, just feels like we might be chasing bugs in this area for a bit.

Copy link
Member

@lateralusX lateralusX Mar 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, fix was evaluating if it was the root cause of the ARM 32-bit issue seen on CoreCLR. Second I didn't want to switch to copy semantics in all cases, since it has been like that for a long time (inherited from C++ codebase) and its an optimization that can be used in most cases of current parsing logic (all except in the collect trace 2). Switching to copy semantics would also change the ownership rules of parsed UTF16 strings meaning we would need to do more adjustments in parsing logic artifacts that currently don't have ownership of the UTF16 strings. I added a runtime assert to make sure the version not doing a byte copy, ds_ipc_message_try_parse_string_utf16_t, will enforce that the passed in buffer is property aligned, in order to catch any future unaligned use. I also looked through all existing calls to ds_ipc_message_try_parse_string_utf16_t and they all currently parsed on aligned buffer address (except the fixed collect trace 2 parsing logic) The above function, ipc_message_try_parse_string_utf16_t_byte_array is also made static, so only called from within that source. Doing a copy if necessary logic would even complicate the ownership rules of these strings more.

Since the alignment is based on the parsing logic implemented in functions like eventpipe_collect_tracing2_command_try_parse_payload there is no static test, but as I said above, I added a runtime validation on debug builds, so as long as we have tests covering this (that we apparently didn't have until we updated the Diagnostic Client), we should hit assert on debug builds, if we add new payloads that could trigger the same issue in future.

The actual SIGBUS happening in this case is also rather interesting since you could argue that it is a compiler optimization bug. ARM supports unaligned reads for word and halfword instructions, but not for double word. The code does two word size reads inside UTF, that would have worked if optimizer didn't bundle them in to a LDRD instruction that will need address to be aligned, so optimization moved from a program that would have worked to one that triggers a SIGBUS.

So to sum up, current fix take care of the scenario currently hit with unaligned UTF16 string in IPC message payload. I added an assert to prevent future incorrect use of UTF16 string parsing function using unaligned addresses. Alternative would be to abandon idea to reuse UTF16 string from payload and always take copy, totally doable, but will add more changes to all current *CommandPayload structs, since they will need to switch to owning copy of UTF16 string and make sure they are freed as part of their fini/free implementations.

uint8_t **buffer,
uint32_t *buffer_len,
const uint8_t **string_byte_array,
uint32_t *string_byte_array_len)
{
EP_ASSERT (buffer != NULL);
EP_ASSERT (buffer_len != NULL);
EP_ASSERT (string_byte_array != NULL);
EP_ASSERT (string_byte_array_len != NULL);

bool result = false;

ep_raise_error_if_nok (ds_ipc_message_try_parse_uint32_t (buffer, buffer_len, string_byte_array_len));
*string_byte_array_len *= sizeof (ep_char16_t);

if (*string_byte_array_len != 0) {
if (*string_byte_array_len > *buffer_len)
ep_raise_error ();

if (((const ep_char16_t *)*buffer) [(*string_byte_array_len / sizeof (ep_char16_t)) - 1] != 0)
ep_raise_error ();

*string_byte_array = *buffer;

} else {
*string_byte_array = NULL;
}

*buffer = *buffer + *string_byte_array_len;
*buffer_len = *buffer_len - *string_byte_array_len;

result = true;

ep_on_exit:
return result;

ep_on_error:
EP_ASSERT (!result);
ep_exit_error_handler ();
}

DiagnosticsIpcMessage *
ds_ipc_message_init (DiagnosticsIpcMessage *message)
{
Expand Down Expand Up @@ -383,38 +435,35 @@ ds_ipc_message_try_parse_uint32_t (
return result;
}

// TODO: Strings are in little endian format in buffer.
bool
ds_ipc_message_try_parse_string_utf16_t (
ds_ipc_message_try_parse_string_utf16_t_byte_array_alloc (
uint8_t **buffer,
uint32_t *buffer_len,
const ep_char16_t **value)
uint8_t **string_byte_array,
uint32_t *string_byte_array_len)
{
EP_ASSERT (buffer != NULL);
EP_ASSERT (buffer_len != NULL);
EP_ASSERT (value != NULL);
EP_ASSERT (string_byte_array != NULL);
EP_ASSERT (string_byte_array_len != NULL);

bool result = false;

uint32_t string_len = 0;
ep_raise_error_if_nok (ds_ipc_message_try_parse_uint32_t (buffer, buffer_len, &string_len));
const uint8_t *temp_buffer = NULL;
uint32_t temp_buffer_len = 0;

if (string_len != 0) {
if (string_len > (*buffer_len / sizeof (ep_char16_t)))
ep_raise_error ();

if (((const ep_char16_t *)*buffer) [string_len - 1] != 0)
ep_raise_error ();
ep_raise_error_if_nok (ipc_message_try_parse_string_utf16_t_byte_array (buffer, buffer_len, (const uint8_t **)&temp_buffer, &temp_buffer_len));

*value = (ep_char16_t *)*buffer;
if (temp_buffer_len != 0) {
*string_byte_array = ep_rt_byte_array_alloc (temp_buffer_len);
ep_raise_error_if_nok (*string_byte_array != NULL);

memcpy (*string_byte_array, temp_buffer, temp_buffer_len);
} else {
*value = NULL;
*string_byte_array = NULL;
}

*buffer = *buffer + (string_len * sizeof (ep_char16_t));
*buffer_len = *buffer_len - (string_len * sizeof (ep_char16_t));

*string_byte_array_len = temp_buffer_len;
result = true;

ep_on_exit:
Expand All @@ -425,6 +474,21 @@ ds_ipc_message_try_parse_string_utf16_t (
ep_exit_error_handler ();
}

bool
ds_ipc_message_try_parse_string_utf16_t (
uint8_t **buffer,
uint32_t *buffer_len,
const ep_char16_t **value)
{
EP_ASSERT (buffer != NULL);
EP_ASSERT (buffer_len != NULL);
EP_ASSERT (value != NULL);
EP_ASSERT (!(((size_t)*buffer) & 0x1));

uint32_t string_byte_array_len = 0;
return ipc_message_try_parse_string_utf16_t_byte_array (buffer, buffer_len, (const uint8_t **)value, &string_byte_array_len);
}

bool
ds_ipc_message_initialize_header_uint32_t_payload (
DiagnosticsIpcMessage *message,
Expand Down
7 changes: 7 additions & 0 deletions src/native/eventpipe/ds-protocol.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,13 @@ ds_ipc_message_try_parse_int32_t (
return ds_ipc_message_try_parse_uint32_t (buffer, buffer_len, (uint32_t *)value);
}

bool
ds_ipc_message_try_parse_string_utf16_t_byte_array_alloc (
uint8_t **buffer,
uint32_t *buffer_len,
uint8_t **string_byte_array,
uint32_t *string_byte_array_len);

bool
ds_ipc_message_try_parse_string_utf16_t (
uint8_t **buffer,
Expand Down
1 change: 0 additions & 1 deletion src/tests/Common/test_dependencies/test_dependencies.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
</PropertyGroup>
<ItemGroup>
<PackageReference Include="Microsoft.Diagnostics.Tools.RuntimeClient" Version="$(MicrosoftDiagnosticsToolsRuntimeClientVersion)" />
<PackageReference Include="Microsoft.Diagnostics.NETCore.Client" Version="$(MicrosoftDiagnosticsNETCoreClientVersion)" />
</ItemGroup>

<Target Name="Build" DependsOnTargets="$(TraversalBuildDependsOn)" />
Expand Down
1 change: 1 addition & 0 deletions src/tests/Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@

<TestBuildMode Condition="'$(__TestBuildMode)' != ''">$(__TestBuildMode)</TestBuildMode>
<RuntimeFlavor Condition="'$(__RuntimeFlavor)' != ''">$(__RuntimeFlavor)</RuntimeFlavor>
<RuntimeFlavor Condition="'$(RuntimeFlavor)' == ''">coreclr</RuntimeFlavor>

<RestoreDefaultOptimizationDataPackage Condition="'$(RestoreDefaultOptimizationDataPackage)' == ''">false</RestoreDefaultOptimizationDataPackage>
<PortableBuild Condition="'$(PortableBuild)' == ''">true</PortableBuild>
Expand Down
11 changes: 5 additions & 6 deletions src/tests/JIT/Directed/debugging/debuginfo/tester.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
using System.Linq;
using System.Reflection;
using System.Runtime.CompilerServices;
using Microsoft.Diagnostics.Tools.RuntimeClient;
using Microsoft.Diagnostics.NETCore.Client;
using Microsoft.Diagnostics.Tracing;
using Microsoft.Diagnostics.Tracing.Parsers;
using Microsoft.Diagnostics.Tracing.Parsers.Clr;
Expand All @@ -23,18 +23,17 @@ public static unsafe int Main()
var keywords =
ClrTraceEventParser.Keywords.Jit | ClrTraceEventParser.Keywords.JittedMethodILToNativeMap;

var dotnetRuntimeProvider = new List<Provider>
var dotnetRuntimeProvider = new List<EventPipeProvider>
{
new Provider("Microsoft-Windows-DotNETRuntime", eventLevel: EventLevel.Verbose, keywords: (ulong)keywords)
new EventPipeProvider("Microsoft-Windows-DotNETRuntime", eventLevel: EventLevel.Verbose, keywords: (long)keywords)
};

var config = new SessionConfiguration(1024, EventPipeSerializationFormat.NetTrace, dotnetRuntimeProvider);

return
IpcTraceTest.RunAndValidateEventCounts(
new Dictionary<string, ExpectedEventCount>(),
JitMethods,
config,
dotnetRuntimeProvider,
1024,
ValidateMappings);
}

Expand Down
2 changes: 2 additions & 0 deletions src/tests/JIT/Directed/debugging/debuginfo/tester.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,15 @@
<Optimize>True</Optimize>
<AllowUnsafeBlocks>True</AllowUnsafeBlocks>
<UnloadabilityIncompatible>true</UnloadabilityIncompatible>
<CLRTestTargetUnsupported Condition="'$(TargetOS)' == 'Linux' And '$(TargetArchitecture)' == 'arm' And '$(RuntimeFlavor)' == 'coreclr'">true</CLRTestTargetUnsupported>
<GCStressIncompatible>true</GCStressIncompatible>
</PropertyGroup>
<ItemGroup>
<ProjectReference Include="tests_d.ilproj" Aliases="tests_d" />
<ProjectReference Include="tests_r.ilproj" Aliases="tests_r" />
<ProjectReference Include="attribute.csproj" />
<ProjectReference Include="../../../../tracing/eventpipe/common/common.csproj" />
<ProjectReference Include="../../../../tracing/eventpipe/common/Microsoft.Diagnostics.NETCore.Client/Microsoft.Diagnostics.NETCore.Client.csproj" />
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>
Loading