Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Match xplat event source conditions #56435

Merged
merged 2 commits into from
Aug 2, 2021
Merged

Match xplat event source conditions #56435

merged 2 commits into from
Aug 2, 2021

Conversation

am11
Copy link
Member

@am11 am11 commented Jul 27, 2021 •
edited
Loading

  • Match the condition in props with:

    runtime/src/coreclr/clrdefinitions.cmake

    Lines 123 to 125 in 2a3141a

    if(CLR_CMAKE_TARGET_LINUX)
    add_definitions(-DFEATURE_EVENTSOURCE_XPLAT)
    endif(CLR_CMAKE_TARGET_LINUX)
  • Add consistency check that prints the class and method names with debug configuration to spot easily what went wrong with COMPlus_ConsistencyCheck=1 (rather than just the id != 0 assertion failure).

Fixes #56079

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 27, 2021
@am11
Copy link
Member Author

am11 commented Jul 27, 2021 •
edited
Loading

cc @josalem, @noahfalk

I'm personally not super excited about fixing it this way, as I think it should be possible to make managed implementation provider-agnostic without the need for constant + preprocessor directives: ETW, LTTng or Dummy. However, since I don't know the rationale, I am deferring to this naive fix. :)

@am11 am11 mentioned this pull request Jul 28, 2021
Closed
@Thefrank
Copy link
Contributor

just tested using preview 6 and this pull request as patch: this fixes the FreeBSD segfault issue!
(the segfault from an sdk change is a different issue I still need to look into)

@Thefrank Thefrank mentioned this pull request Jul 29, 2021
Merged
@am11
Copy link
Member Author

am11 commented Jul 29, 2021

@Thefrank, thanks for confirming.

I was reading about "how to instrument dtrace probes" from the perspective of requirements in coreclr and mono. I also looked into dtrace support on other (non SunOS-likes) systems such as FreeBSD, and Windows (https://github.com/microsoft/DTrace-on-Windows). I think after 6.0 is released, we can discuss it with .NET Tracing team and implement an additional provider for Dtrace, that will help platforms which primarily prefer Dtrace over LTTng or other solutions (illumos, FreeBSD etc.).

@josalem
Copy link
Contributor

josalem commented Jul 29, 2021

The change looks good to me, but I want to make sure that this isn't regressing LTTng support on accident. (I don't think it is, but we don't have an automated test for that that I'm aware of)

wfurt pushed a commit to dotnet/arcade that referenced this pull request Jul 30, 2021
@Thefrank
add terminfo-db to FreeBSD package requirements (#7588)
093cd72 
* add terminfo-db to FreeBSD package requirements

* runtime can use terminfo under FreeBSD but only with this additional package
* from feedback: dotnet/runtime#55152 (comment)

* fix typo, bump FreeBSD12, remove lttng-ust from it

lttng-ust removable depends on dotnet/runtime#56435 merge

* lttng-ust needs to stay

* add FreeBSD13, add support for FreeBSD ABI

also: use -j $JOBS during build while we are here
@am11
Copy link
Member Author

am11 commented Aug 2, 2021

we don't have an automated test for that that I'm aware of

Would you like me to add a test for it? AFAICT:

  • if platform has a managed implementation of LTTng but missing the native provider, the existing assertion in ecall.cpp (_ASSERTE(id != 0)) will fail in Debug and Checked legs, because it happens during the CLR startup which fails even a simple hello-world app.
  • LTTng is Linux-only in native build, this PR is updating a condition for managed build to match native config: != "OSX" to == "Linux".

josalem
josalem approved these changes Aug 2, 2021
View reviewed changes
Copy link
Contributor

@josalem josalem left a comment

Choose a reason for hiding this comment

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

Would you like me to add a test for it?

We've been wanting to add a test for LTTng for a while. If you create one, I will review it 😄.

I don't see a reason to delay this PR on that though. I'm confident that this shouldn't affect LTTng functionality based on the reasons you laid out. I'll still do some manual testing due to where we are in the release cycle though.

@josalem
Copy link
Contributor

josalem commented Aug 2, 2021

CC @brianrob are you aware of any platforms that will be excluded by this change that support LTTng? As far as I can tell, there aren't any in the support matrix.

@brianrob
Copy link
Member

brianrob commented Aug 2, 2021

CoreCLR has only ever supported LTTng on Linux. At one point, I saw some changes going into the LTTng source base to potentially support OSX, but I don't know where that is at this point. If we did want to support that, we'd need to make more changes to the runtime anyway, so this seems totally fine.

@josalem josalem merged commit f83a9d9 into dotnet:main Aug 2, 2021
@josalem
Copy link
Contributor

josalem commented Aug 2, 2021

Thanks for the contribution @am11 😄

@am11 am11 deleted the fix/illumos-execution branch August 2, 2021 20:12
@Thefrank
Copy link
Contributor

Thefrank commented Aug 2, 2021

FreeBSD uses LTTng but its not in the official matrix.
Platforms like FreeBSD (BSD-like) also prefer something like DTrace but that is a discussion for much later

@josalem
Copy link
Contributor

josalem commented Aug 2, 2021

Platforms like FreeBSD (BSD-like) also prefer something like DTrace

Out of curiosity, does DTrace use UDSTs (User Defined Static Tracepoints) to hook into user code?

@am11
Copy link
Member Author

am11 commented Aug 2, 2021

Out of curiosity, does DTrace use UDSTs (User Defined Static Tracepoints) to hook into user code?

From OpenDTrace v1.0 docs: https://www.cl.cam.ac.uk/techreports/UCAM-CL-TR-924.pdf

OpenDTrace has a base set of providers that are shipped as part of the system, but devel-
opers are free to create their own, to expose more or different information from their code.
Providers can be developed either for the kernel, in which case they are defined as kernel mod-
ules, or for user space, as part of the Userland Statically Defined Tracing (USDT) system.

@josalem
Copy link
Contributor

josalem commented Aug 2, 2021

Cool! Good to know. I believe we instrumented most of the native runtime events to be USDT (or UDST) back when we did the LTTng bring up, so theoretically DTrace should already work with a good chunk of the GC and JIT events. Managed events are a whole different discussion though.

thaystg added a commit to thaystg/runtime that referenced this pull request Aug 3, 2021
@thaystg
Merge remote-tracking branch 'origin/main' into thays_fix_56249
c0e7fb2 
* origin/main: (64 commits)
  [wasm][debugger] Create test Inherited Properties (dotnet#56754)
  Mark new test as incompatible with GC Mark4781_1GcStressIncompatible (dotnet#56739)
  Ensure MetadataEnumResult is sufficiently updated by MetaDataImport::Enum (dotnet#56756)
  [mono] Remove gdb xdebug and binary writer support, it hasn't worked in a while. (dotnet#56759)
  Update windows-requirements.md (dotnet#56476)
  Update doc and generic parameter name for JsonValue.GetValue (dotnet#56639)
  [wasm][debugger] Inspect static class (dotnet#56740)
  Fix stack overflow handling issue in GC stress (dotnet#56733)
  Use ReflectionOnly as serialization mode in case dynamic code runtime feature is not supported (dotnet#56604)
  Move Windows Compat pack to NuGet pack task (dotnet#56686)
  Fix build error when building some packages (dotnet#56767)
  Simplify JIT shutdown logic in crossgen2 (dotnet#56687)
  Fix race in crossdac publishing with PGO (dotnet#56762)
  Add DictionaryKeyPolicy support for EnumConverter [dotnet#47765] (dotnet#54429)
  Use ComWrappers in some Marshal unit-tests and update platform metadata  (dotnet#56595)
  Set `DisableImplicitNamespaceImports_Dotnet=true` to workaround sdk issue (dotnet#56744)
  Make sure ServerGCHeapDetails is up to date (dotnet#56056)
  [libraries] Reenable System.Diagnostics.DiagnosticSorce.Switches.Tests on mobile (dotnet#56737)
  Disable failing arm64 win10 Graphics.FromHdc tests  (dotnet#56732)
  Match xplat event source conditions (dotnet#56435)
  ...
@ghost ghost locked as resolved and limited conversation to collaborators Sep 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@josalem josalem josalem approved these changes

Assignees
No one assigned
Labels
area-Tracing-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Relationship between FEATURE_EVENT_TRACE and FEATURE_EVENTSOURCE_XPLAT
4 participants
@am11 @Thefrank @josalem @brianrob