Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Revert "Add environment variable (COMPlus_EnableDiagnostics) to disab… #16007

Merged
merged 1 commit into from
Jan 25, 2018
Merged

Revert "Add environment variable (COMPlus_EnableDiagnostics) to disab… #16007

merged 1 commit into from
Jan 25, 2018

Conversation

RussKeldorph
Copy link

…le debugging and profiling. (#15878)"

This reverts commit 5bcfde4.

If the armel CI jobs don't hang on this, then that suggests #15878 is responsible for #15914.

@mikem8361 FYI

@RussKeldorph
Revert "Add environment variable (COMPlus_EnableDiagnostics) to disab…
8e42b4c 
…le debugging and profiling. (#15878)"

This reverts commit 5bcfde4.
@RussKeldorph RussKeldorph added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Jan 25, 2018
jkotas
jkotas reviewed Jan 25, 2018
View reviewed changes
src/debug/ee/debugger.cpp
@@ -2085,6 +2103,22 @@ HRESULT Debugger::Startup(void)

RaiseStartupNotification();

// Also initialize the AppDomainEnumerationIPCBlock
Copy link
Member

Choose a reason for hiding this comment

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

@mikem8361 Notice that FEATURE_DBGIPC_TRANSPORT_VM block above uses m_pAppDomainCB - that is initialized too late now.

Copy link

Choose a reason for hiding this comment

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

My change that is being reverted moved the m_pAppDomainCB initialization to almost the beginning of Debugger::Startup() so it should be initialized.

The change is moving the m_AppDomainCB back to where it was (later in Startup()).

Am I missing something?

Copy link
Member

@jkotas jkotas Jan 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, I go it backward ... you are right. But I do think that the change of the order here caused the regression somehow.

Copy link
Member

@jkotas jkotas Jan 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

And it is likely that we have the same regression on other Linux flavors and just do not see it as often. The ARM emulator is a lot slower than a real machine, so it hits races that are hard to hit elsewhere.

@benaadams
Copy link
Member

Sort of worked; but not reporting success to CI

Perform an action if the job was performed on an Azure VM Agent. is waiting for a checkpoint on 
dotnet_coreclr » master » armel_cross_checked_tizen_prtest #2999
[Current build status] check if current [SUCCESS] is worse or equals then [SUCCESS] and better or equals then [SUCCESS]
Run condition [Current build status] enabling perform for step [[Archive the artifacts]]
Archiving artifacts
Notifying endpoint with credentials id 'helix-int-notification-url'
Notifying endpoint with credentials id 'helix-prod-notification-url'
Setting status of 8e42b4cdba2607956477336d6bc3ac2aa0cd57c4 to SUCCESS with url https://ci.dot.net/job/dotnet_coreclr/job/master/job/armel_cross_checked_tizen_prtest/3000/ and message: 'Build finished. '
Using context: Tizen armel Cross Checked Build
[WS-CLEANUP] Deleting project workspace...Cannot delete workspace
 :remote file operation failed:
 /mnt/j/workspace/dotnet_coreclr/master/armel_cross_checked_tizen_prtest at hudson.remoting.Channel@6e0ba86f:ubuntu1404-20170120-9d4ad3:
 java.io.IOException: Unable to delete '/mnt/j/workspace/dotnet_coreclr/master/armel_cross_checked_tizen_prtest/Tools/15.0/Microsoft.Common.props'.
 Tried 3 times (of a maximum of 3) waiting 0.1 sec between attempts.
ERROR: Step ‘Delete workspace when build is done’ failed:
 Cannot delete workspace:
 remote file operation failed:
 /mnt/j/workspace/dotnet_coreclr/master/armel_cross_checked_tizen_prtest at hudson.remoting.Channel@6e0ba86f:ubuntu1404-20170120-9d4ad3:
 java.io.IOException: Unable to delete '/mnt/j/workspace/dotnet_coreclr/master/armel_cross_checked_tizen_prtest/Tools/15.0/Microsoft.Common.props'.
 Tried 3 times (of a maximum of 3) waiting 0.1 sec between attempts.
Finished: SUCCESS

@mikem8361
Copy link

The error you hit seems to be a build infrastructure problem.

I really hope we won't have to merge this reversion. I don't see how this could affect only Linux Arm tests. I don't know how to debug this environment .

@benaadams
Copy link
Member

CentOS7.1 x64 Release Innerloop Build and Test seems infrastructure

First time build. Skipping changelog.
parallel {
    Schedule job dotnet_coreclr » master » x64_release_centos7.1_innerloop_prtest
    Build dotnet_coreclr » master » x64_release_centos7.1_innerloop_prtest #12 started
    dotnet_coreclr » master » x64_release_centos7.1_innerloop_prtest #12 completed 
ERROR: Failed to run DSL Script
java.util.concurrent.ExecutionException:
dbees.plugins.flow.JobNotFoundException:
net_coreclr/master/x64_release_windows_nt_innerloop_bld_prtest not found (or isn't a job).
	at java.util.concurrent.FutureTask.report(FutureTask.java:122)
	at java.util.concurrent.FutureTask.get(FutureTask.java:192)
	at java_util_concurrent_Future$get.call(Unknown Source)
	at com.cloudbees.plugins.flow.FlowDelegate$_parallel_closure6.doCall(FlowDSL.groovy:456)
	at sun.reflect.GeneratedMethodAccessor1199.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.codehaus.groovy.reflection.CachedMethod.invoke(CachedMethod.java:93)
	at groovy.lang.MetaMethod.doMethodInvoke(MetaMethod.java:325)
	at org.codehaus.groovy.runtime.metaclass.ClosureMetaClass.invokeMethod(ClosureMetaClass.java:294)
	at groovy.lang.MetaClassImpl.invokeMethod(MetaClassImpl.java:1022)
	at groovy.lang.Closure.call(Closure.java:414)
	at groovy.lang.Closure.call(Closure.java:430)

@mikem8361
Copy link

mikem8361 commented Jan 25, 2018 via email

@jkotas
Copy link
Member

jkotas commented Jan 25, 2018

I don't see how this could affect only Linux Arm tests.

There is a strong evidence that it did affect Linux Arm tests. They started consistently failing right after your change went in; and this trial revert made them pass again.

@RussKeldorph
Copy link
Author

I really hope we won't have to merge this reversion. I don't see how this could affect only Linux Arm tests. I don't know how to debug this environment .

@mikem8361 I'm not insisting that merging this is the right fix. I just wanted to demonstrate that #15878 is the likely culprit. I think @jashook or @Samsung can help get you set up to debug in this environment if you can't figure it out with @jkotas thoughts. I would ask that you make it your top priority to investigate this and consider merging this PR if you can't, however.

@mikem8361
Copy link

mikem8361 commented Jan 25, 2018 via email

benaadams
benaadams reviewed Jan 25, 2018
View reviewed changes
src/debug/ee/debugger.cpp
@@ -7186,8 +7220,7 @@ void Debugger::JitAttach(Thread * pThread, EXCEPTION_POINTERS * pExceptionInfo,
}
CONTRACTL_END;

// Don't do anything if there is a native debugger already attached or the debugging support has been disabled.
if (IsDebuggerPresent() || m_pRCThread == NULL)
Copy link
Member

Choose a reason for hiding this comment

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

Aside, should this be reversed?

if (m_pRCThread == NULL || IsDebuggerPresent())

@mikem8361
Copy link

mikem8361 commented Jan 25, 2018 via email

@RussKeldorph RussKeldorph removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Jan 25, 2018
@RussKeldorph RussKeldorph changed the title [WIP] Revert "Add environment variable (COMPlus_EnableDiagnostics) to disab… Revert "Add environment variable (COMPlus_EnableDiagnostics) to disab… Jan 25, 2018
@RussKeldorph
Copy link
Author

CentOS Release failure is #16016. Merging per above. We can debug as time allows and make another pull request when ready. @lt72 FYI

@RussKeldorph RussKeldorph merged commit e998512 into dotnet:master Jan 25, 2018
@RussKeldorph RussKeldorph deleted the revert15878 branch January 25, 2018 19:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@benaadams benaadams benaadams left review comments

@jkotas jkotas jkotas left review comments

@mikem8361 mikem8361 mikem8361 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@RussKeldorph @benaadams @mikem8361 @jkotas