Redefine signal for architecture mips #40299

sunny868 · 2020-08-04T02:08:04Z

define SIGSTOP 19 is just ok for architecture x86、arm、s390、powerpc and so on.
but others architecture has different values.
For architecture mips,you can see kernel source code arch/mips/include/uapi/asm/signal.h

Dotnet-GitSync-Bot · 2020-08-04T02:08:07Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

dnfadmin · 2020-08-04T02:08:18Z

All CLA requirements met.

sunny868 · 2020-08-04T02:48:35Z

I'm not sure it's the best way to redefine SIGSTOP for architecture mips in Interop.Kill.cs. Maybe we can redefine SIGSTOP in pal_process.c :

_int32_t SystemNative_Kill(int32_t pid, int32_t signal)
{
#ifdef __TARGET_MIPS64_
if( signal == 19)
signal = SIGSTOP;
#endif
return kill(pid, signal);
}

sunny868 · 2020-08-04T04:30:01Z

The 4 failing has nothing to do with this PR.

src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj

define SIGSTOP 19 is just ok for architecture x86、arm、s390、powerpc and so on. but others architecture has different values. For architecture mips,you can see kernel source code arch/mips/include/uapi/asm/signal.h

src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj

ghost · 2020-08-04T14:31:24Z

Tagging subscribers to this area: @eiriktsarpalis
See info in area-owners.md if you want to be subscribed.

src/libraries/Common/src/Interop/Unix/System.Native/Interop.Kill.cs

define SIGSTOP 19 is just ok for architecture x86、arm、s390、powerpc and so on. but others architecture has different values. For architecture mips,you can see kernel source code arch/mips/include/uapi/asm/signal.h Co-Authored-By: hev <[email protected]>

jkotas · 2020-08-05T03:14:41Z

src/libraries/Native/Unix/System.Native/pal_process.c

I would be better to define PAL_... specific enum in pal_process.h (similar to other enums that exist there already). This enum should have just the few signals that we actually need, and this remapping can then be done unconditionally, without hardcoding of magic numbers.

I'm worried that more signals will be added in Interop.Kill.cs (internal enum Signals:int{..}) later.

If that happens, both places will need to be updated. It is same with all other PAL enums.

This reverts commit fcc2d46.

jkotas · 2020-08-05T14:14:27Z

src/libraries/Native/Unix/System.Native/pal_process.c

Suggested change

signal = SIGSTOP;

}

signal = SIGSTOP;

break;

}

jkotas · 2020-08-05T14:22:04Z

src/libraries/Native/Unix/System.Native/pal_process.c

Maybe this can even be:

switch (signal) { case PAL_SIGKILL: signal = SIGKILL; break; case PAL_SIGSTOP: signal = SIGSTOP; break; default: assert_msg(false, "Unknown signal"); break; }

And delete c_static_assert(PAL_SIGKILL == SIGKILL); at the top of the file. I have verified that clang optimizes the switch out when the remap-signal numbers match.

define SIGSTOP 19 is just ok for architecture x86、arm、s390、powerpc and so on. but others architecture has different values. For architecture mips,you can see kernel source code arch/mips/include/uapi/asm/signal.h Co-Authored-By: hev <[email protected]>

sunny868 · 2020-08-06T04:55:13Z

Maybe The 11 failing has nothing to do with this PR.

jkotas

Thanks

safern · 2020-08-06T16:43:26Z

@jkotas it seems like this PR was merged with System.Diagnostics.Process test failures hitting the assert that was added as part of this PR:

/root/helix/work/workitem /root/helix/work/workitem
  Discovering: System.Diagnostics.Process.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Diagnostics.Process.Tests (found 235 of 291 test cases)
  Starting:    System.Diagnostics.Process.Tests (parallel test collections = on, max threads = 2)
    System.Diagnostics.Tests.ProcessStartInfoTests.ShellExecute_Nano_Fails_Start [SKIP]
      Condition(s) not met: "IsWindowsNanoServer"
Linux
Linux
Linux
Linux
None of the following programs were installed on this machine: xdg-open,gnome-open,kfmclient.
/__w/1/s/src/libraries/Native/Unix/System.Native/pal_process.c (665): error 0: Unknown signal (false failed)
Assertion failed: false && "assert_msg failed" (/__w/1/s/src/libraries/Native/Unix/System.Native/pal_process.c: SystemNative_Kill: 665)
./RunTests.sh: line 161:    24 Aborted                 (core dumped) "$RUNTIME_PATH/dotnet" exec --runtimeconfig System.Diagnostics.Process.Tests.runtimeconfig.json --depsfile System.Diagnostics.Process.Tests.deps.json xunit.console.dll System.Diagnostics.Process.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing $RSP_FILE
/root/helix/work/workitem

This reverts commit 0f7370b.

safern · 2020-08-06T16:46:27Z

I put up a revert here: #40470

jkotas · 2020-08-06T16:54:41Z

I am sorry. Once I have seen the known problem "CannotRemoveAttribute : Attribute 'System.ComponentModel.TypeConverterAttribute' exists on 'System.ComponentModel.MarshalByValueComponent' " , I have stopped looking through all the failures.

jkotas · 2020-08-06T16:55:13Z

@sunny868 Could you please resubmit with the fix?

safern · 2020-08-06T16:56:21Z

I am sorry. Once I have seen the known problem "CannotRemoveAttribute : Attribute 'System.ComponentModel.TypeConverterAttribute' exists on 'System.ComponentModel.MarshalByValueComponent' " , I have stopped looking through all the failures.

Yeah it is understandable when CI is on the floor because of other things, that things like this could slip through. I'm working on fixing the ApiCompat problem.

This reverts commit 0f7370b.

sunny868 · 2020-08-06T22:51:28Z

@sunny868 Could you please resubmit with the fix?

Sorry, I I forgot to reset value "Interop.Sys.Signals.None" to PAL_NONE . I'll resubmit it later.

sunny868 · 2020-08-07T09:02:14Z

@jkotas I have resubmit it ,please review : #40513

define SIGSTOP 19 is just ok for architecture x86、arm、s390、powerpc and so on. but others architecture has different values. Co-Authored-By: hev <[email protected]>

…#40470) This reverts commit 0f7370b.

am11 reviewed Aug 4, 2020

View reviewed changes

src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj Outdated Show resolved Hide resolved

sunny868 force-pushed the mips_branch branch from 0269c1e to 16cc7f2 Compare August 4, 2020 05:58

Define SIGSTOP 23 for architecture mips

fcc2d46

define SIGSTOP 19 is just ok for architecture x86、arm、s390、powerpc and so on. but others architecture has different values. For architecture mips,you can see kernel source code arch/mips/include/uapi/asm/signal.h

sunny868 force-pushed the mips_branch branch from 16cc7f2 to fcc2d46 Compare August 4, 2020 06:02

jkotas reviewed Aug 4, 2020

View reviewed changes

src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj Outdated Show resolved Hide resolved

jkotas added the area-System.Diagnostics.Process label Aug 4, 2020

jkotas reviewed Aug 5, 2020

View reviewed changes

src/libraries/Common/src/Interop/Unix/System.Native/Interop.Kill.cs Outdated Show resolved Hide resolved

jkotas reviewed Aug 5, 2020

View reviewed changes

sunny868 force-pushed the mips_branch branch from 93f67d1 to a05f1d6 Compare August 5, 2020 03:17

Revert " Define SIGSTOP 23 for architecture mips"

16bb958

This reverts commit fcc2d46.

jkotas reviewed Aug 5, 2020

View reviewed changes

sunny868 force-pushed the mips_branch branch from 40e14ed to efb8515 Compare August 6, 2020 00:53

sunny868 force-pushed the mips_branch branch from efb8515 to 298c6ec Compare August 6, 2020 01:34

jkotas approved these changes Aug 6, 2020

View reviewed changes

jkotas merged commit 0f7370b into dotnet:master Aug 6, 2020

sunny868 deleted the mips_branch branch August 6, 2020 06:20

safern added a commit that referenced this pull request Aug 6, 2020

Revert " Define SIGSTOP for architecture mips (#40299)"

d15a8ae

This reverts commit 0f7370b.

safern mentioned this pull request Aug 6, 2020

Revert " Define SIGSTOP 23 for architecture mips" #40470

Merged

safern added a commit that referenced this pull request Aug 6, 2020

Revert " Define SIGSTOP for architecture mips (#40299)" (#40470)

0adf9b5

This reverts commit 0f7370b.

sunny868 changed the title ~~Define SIGSTOP 23 for architecture mips~~ Redefine signal for architecture mips Aug 7, 2020

Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020

Revert " Define SIGSTOP for architecture mips (dotnet#40299)" (dotnet…

c393da2

…#40470) This reverts commit 0f7370b.

karelz added this to the 5.0.0 milestone Aug 18, 2020

ghost locked as resolved and limited conversation to collaborators Dec 7, 2020

-             signal = SIGSTOP;
-    }
+             signal = SIGSTOP;
+             break;
+    }

Redefine signal for architecture mips #40299

Redefine signal for architecture mips #40299

Uh oh!

Conversation

sunny868 commented Aug 4, 2020

Uh oh!

Dotnet-GitSync-Bot commented Aug 4, 2020

Uh oh!

dnfadmin commented Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sunny868 commented Aug 4, 2020

Uh oh!

sunny868 commented Aug 4, 2020

Uh oh!

Uh oh!

Uh oh!

ghost commented Aug 4, 2020

Uh oh!

Uh oh!

jkotas Aug 5, 2020

Choose a reason for hiding this comment

Uh oh!

sunny868 Aug 5, 2020

Choose a reason for hiding this comment

Uh oh!

jkotas Aug 5, 2020

Choose a reason for hiding this comment

Uh oh!

sunny868 Aug 5, 2020

Choose a reason for hiding this comment

Uh oh!

jkotas Aug 5, 2020

Choose a reason for hiding this comment

Uh oh!

jkotas Aug 5, 2020

Choose a reason for hiding this comment

Uh oh!

sunny868 Aug 6, 2020

Choose a reason for hiding this comment

Uh oh!

sunny868 commented Aug 6, 2020

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

safern commented Aug 6, 2020

Uh oh!

safern commented Aug 6, 2020

Uh oh!

jkotas commented Aug 6, 2020

Uh oh!

jkotas commented Aug 6, 2020

Uh oh!

safern commented Aug 6, 2020

Uh oh!

sunny868 commented Aug 6, 2020

Uh oh!

sunny868 commented Aug 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dnfadmin commented Aug 4, 2020 •

edited

Loading