Enhance coreclr processing of event pipe configuration to be able to write the pid of the current process into the nettrace file #48537
Conversation
davidwrighton
commented
Feb 19, 2021
davidwrighton
commented
- Also fix the behavior of get_next_config_value_as_utf8_string to work correctly on coreclr, it was type-punning between a buffer returned by ep_rt_byte_array_alloc and ep_rt_utf8_string_free, which isn't safe in the coreclr implementation
…write the pid of the current process into the nettrace file - Also fix the behavior of get_next_config_value_as_utf8_string to work correctly on coreclr, it was type-punning between a buffer returned by ep_rt_byte_array_alloc and ep_rt_utf8_string_free, which isn't safe in the coreclr implementation
ghost
added
the
lateralusX
left a comment
lateralusX
left a comment
There was a problem hiding this comment.
Initially we didn't have a need for a pure UTF8 string alloc function and didn't add it, mainly since it caused some confusion around byte vs number of characters as the argument when calling alloc and we tried to stay away as much as possible regarding length of UTF8 strings (there is no ep_rt_utf8_string_len for example) and try to fallback to using len -1 when doing conversions between UTF8 and UTF16.
We have a ep_rt_utf8_string_dup function for null terminated strings maybe an alternative could be to add a range based dup function for UTF8 strings (start/end) that could be used in get_next_config_value_as_utf8_string, pretty much doing ptrdiff + 1 using runtimes string allocator (malloc for example) and then doing a memcpy and NULL termination if needed).
josalem
left a comment
josalem
left a comment
There was a problem hiding this comment.
This seems like a good addition to this activation mechanism. This change should also be documented here before it ships in a preview.
Could you add a CI test for the PID insertion to make sure it's working across all supported platforms?
| ep_char8_t* outputPathPid = strstr(outputPath, pidSearchString); | ||
| if (outputPathPid != NULL) | ||
| { | ||
| size_t newBufSize = strlen(outputPath) + 12; // 12 is 1 for the null-terminator + 11 for the largest possible 4 byte integer converted to a string |
There was a problem hiding this comment.
If I recall, macOS uses 64-bit PIDs. Does GetCurrentProcessId account for that or does it always truncate to a 32-bit value?
There was a problem hiding this comment.
@janvorli Do I need to worry about 64bit pids? I was assuming that the PAL would insulate me here, but if some supported OS might actually use a 64bit pid one could get in trouble. Do you have any data here?
|
@lateralusX I've addressed your feedback, I like the range based dup. @josalem, Do we have any existing CI tests which test this sort of environment variable in our current CI that you are aware of? I'd like to follow a pattern if we've done that somewhere. |
|
One though, shouldn't this feature be available in Mono as well, looks like it is currently implemented as part of CoreClr's runtime shim, meaning it won't work for Mono, but don't see why it couldn't. |
| string tempDir = Path.GetDirectoryName(typeof(NameConfigWithPid).Assembly.Location); | ||
| string outputPathPattern = Path.Combine(tempDir, "eventPipeStream{pid}_{pid}.nettrace"); | ||
|
|
||
| foreach (string oldFile in Directory.EnumerateFiles(tempDir, "eventPipeStream*")) |
There was a problem hiding this comment.
This can be problematic in CI since you may run into multiple instances of this test running concurrently. You may accidentally throw on that Delete or break another test inadvertently. We can avoid this by adding some random values to the end of the filename, e.g., Path.GetRandomFileName.
| { | ||
| Console.WriteLine($"{expectedPath} found"); | ||
| return 100; | ||
| } |
There was a problem hiding this comment.
We should wrap this in a try/finally that attempts to delete the file it potentially created.
You could use the code in the |
4 participants
