Replacing `BOOL` with `bool` #52585

WhiteBlackGoose · 2021-05-11T07:20:47Z

Replaced all Windows' BOOL to bool in RyuJIT except
for WIN API related cases.

Replaced all Windows' BOOL to bool in RyuJIT except for WIN API related cases. Fix dotnet#48317

Replaced `TRUE` with `true` and `FALSE` with `false` constants in RyuJIT except Windows-specific cases.

Formatting patch applied according to the instruction.

WhiteBlackGoose · 2021-05-11T16:09:06Z

src/coreclr/jit/lclvars.cpp

@@ -564,8 +564,7 @@ void Compiler::lvaInitRetBuffArg(InitVarDscInfo* varDscInfo, bool useFixedRetBuf
            info.compCompHnd->getMethodSig(info.compMethodHnd, &sigInfo);
            assert(JITtype2varType(sigInfo.retType) == info.compRetType); // Else shouldn't have a ret buff.

-            info.compRetBuffDefStack =
-                (info.compCompHnd->isStructRequiringStackAllocRetBuf(sigInfo.retTypeClass));
+            info.compRetBuffDefStack = (info.compCompHnd->isStructRequiringStackAllocRetBuf(sigInfo.retTypeClass));
            if (info.compRetBuffDefStack)


Wait, my changes didn't affect this one (it was the suggested patch). CI didn't pass in the main branch, did it? 🦆

what do you mean? I do not think that the jit-format job deleted == TRUE); here.

Ohhh, right, sorry, I'm blind XD. Yes, it's completely alright.

WhiteBlackGoose · 2021-05-11T17:07:01Z

Okay, now it's ready for review. Are those failing CIs - my bad?

hoyosjs · 2021-05-12T03:05:32Z

/azp run runtime

azure-pipelines · 2021-05-12T03:05:58Z

Azure Pipelines successfully started running 1 pipeline(s).

hoyosjs · 2021-05-12T03:06:20Z

@WhiteBlackGoose No, it was a break in the CI orchestrator. I've retriggered the build.

AraHaan

Looks good (if only constexpr was used though for some of these that only returns booleans and safe checks). I know a lot of things in the stl that uses constexpr and noexcept for things like those on these headers and things known within the functions callstacks to never throw.

As such I think a lot of the runtime that has bits that never throw should mark noexcept and constexpr if possible.

WhiteBlackGoose · 2021-05-12T20:24:55Z

@JulieLeeMSFT could you review, please? I'm not sure if I have to ping someone, but no reaction for this and #51758 PR came from members, so I'm a bit confused 😕

JulieLeeMSFT · 2021-05-12T21:34:47Z

@sandreenko PTAL.

JulieLeeMSFT · 2021-05-12T21:39:10Z

@JulieLeeMSFT could you review, please? I'm not sure if I have to ping someone, but no reaction for this and #51758 PR came from members, so I'm a bit confused 😕

@WhiteBlackGoose sorry for the delay. Pinged @sandreenko for code review.

sandreenko · 2021-05-12T21:44:41Z

@JulieLeeMSFT could you review, please? I'm not sure if I have to ping someone, but no reaction for this and #51758 PR came from members, so I'm a bit confused 😕

I recommend you to tag @dotnet/jit-contrib in jit changes when they are ready for review so we get a notification about them. Otherwise, we scroll through all-new PRs from time to time but it would take longer until we notice it.

sandreenko

LGTM, thanks @WhiteBlackGoose!

sandreenko · 2021-05-12T21:47:38Z

src/coreclr/jit/lclvars.cpp

@@ -564,8 +564,7 @@ void Compiler::lvaInitRetBuffArg(InitVarDscInfo* varDscInfo, bool useFixedRetBuf
            info.compCompHnd->getMethodSig(info.compMethodHnd, &sigInfo);
            assert(JITtype2varType(sigInfo.retType) == info.compRetType); // Else shouldn't have a ret buff.

-            info.compRetBuffDefStack =
-                (info.compCompHnd->isStructRequiringStackAllocRetBuf(sigInfo.retTypeClass));
+            info.compRetBuffDefStack = (info.compCompHnd->isStructRequiringStackAllocRetBuf(sigInfo.retTypeClass));
            if (info.compRetBuffDefStack)


what do you mean? I do not think that the jit-format job deleted == TRUE); here.

SingleAccretion · 2021-05-12T22:06:01Z

Red CI seems to be reporting failures, the tests themselves passed.

Libraries Test Run checked coreclr Linux x64 Debug
   System.Runtime.Tests  Total: 38821, Errors: 0, Failed: 0, Skipped: 12, Time: 123.107s

Libraries Test Run checked coreclr Linux_musl arm64 Release
   System.IO.FileSystem.Tests  Total: 5519, Errors: 0, Failed: 0, Skipped: 16, Time: 113.020s

Libraries Test Run checked coreclr Linux_musl x64 Debug
   System.Xml.RW.ReaderSettings.Tests  Total: 926, Errors: 0, Failed: 0, Skipped: 0, Time: 2.649s

Worker 1: got error: Traceback (most recent call last):
  File "/datadisks/disk1/work/B71D09DC/p/reporter/run.py", line 44, in run
    self.__process(item)
  File "/datadisks/disk1/work/B71D09DC/p/reporter/run.py", line 34, in __process
    self.publisher.upload_batch(batch)
  File "/datadisks/disk1/work/B71D09DC/p/reporter/azure_devops_result_publisher.py", line 35, in upload_batch
    self.publish_results(test_case_results, test_name_order, results_with_attachments)
  File "/datadisks/disk1/work/B71D09DC/p/reporter/azure_devops_result_publisher.py", line 70, in publish_results
    published_results = test_client.add_test_results_to_test_run(list(test_case_results), self.team_project, self.test_run_id)  # type: List[TestCaseResult]
  File "/home/helixbot/.azdo-env/lib/python3.6/site-packages/azure/devops/v5_1/test/test_client.py", line 679, in add_test_results_to_test_run
    content=content)
  File "/home/helixbot/.azdo-env/lib/python3.6/site-packages/azure/devops/client.py", line 104, in _send
    response = self._send_request(request=request, headers=headers, content=content, media_type=media_type)
  File "/home/helixbot/.azdo-env/lib/python3.6/site-packages/azure/devops/client.py", line 68, in _send_request
    self._handle_error(request, response)
  File "/home/helixbot/.azdo-env/lib/python3.6/site-packages/azure/devops/client.py", line 272, in _handle_error
    status_code=response.status_code))
azure.devops.exceptions.AzureDevOpsClientRequestError: Operation returned an invalid status code of 503.

AraHaan · 2021-05-12T22:23:09Z

Red CI seems to be reporting failures, the tests themselves passed.

Libraries Test Run checked coreclr Linux x64 Debug
   System.Runtime.Tests  Total: 38821, Errors: 0, Failed: 0, Skipped: 12, Time: 123.107s

Libraries Test Run checked coreclr Linux_musl arm64 Release
   System.IO.FileSystem.Tests  Total: 5519, Errors: 0, Failed: 0, Skipped: 16, Time: 113.020s

Libraries Test Run checked coreclr Linux_musl x64 Debug
   System.Xml.RW.ReaderSettings.Tests  Total: 926, Errors: 0, Failed: 0, Skipped: 0, Time: 2.649s

Worker 1: got error: Traceback (most recent call last):
  File "/datadisks/disk1/work/B71D09DC/p/reporter/run.py", line 44, in run
    self.__process(item)
  File "/datadisks/disk1/work/B71D09DC/p/reporter/run.py", line 34, in __process
    self.publisher.upload_batch(batch)
  File "/datadisks/disk1/work/B71D09DC/p/reporter/azure_devops_result_publisher.py", line 35, in upload_batch
    self.publish_results(test_case_results, test_name_order, results_with_attachments)
  File "/datadisks/disk1/work/B71D09DC/p/reporter/azure_devops_result_publisher.py", line 70, in publish_results
    published_results = test_client.add_test_results_to_test_run(list(test_case_results), self.team_project, self.test_run_id)  # type: List[TestCaseResult]
  File "/home/helixbot/.azdo-env/lib/python3.6/site-packages/azure/devops/v5_1/test/test_client.py", line 679, in add_test_results_to_test_run
    content=content)
  File "/home/helixbot/.azdo-env/lib/python3.6/site-packages/azure/devops/client.py", line 104, in _send
    response = self._send_request(request=request, headers=headers, content=content, media_type=media_type)
  File "/home/helixbot/.azdo-env/lib/python3.6/site-packages/azure/devops/client.py", line 68, in _send_request
    self._handle_error(request, response)
  File "/home/helixbot/.azdo-env/lib/python3.6/site-packages/azure/devops/client.py", line 272, in _handle_error
    status_code=response.status_code))
azure.devops.exceptions.AzureDevOpsClientRequestError: Operation returned an invalid status code of 503.

503 is Unavailable meaning DevOps was offline for some reason. Possibly due to an outage or maintenance.

hoyosjs · 2021-05-12T23:26:21Z

Checked on the failures. There's no failed tests. @sandreenko, if you feel like this is ready to merge please do so.

@MattGal I've seen this issue with 503s starting to cause considerable pain, where we'll fail due to reporting even though all tests passed. Helix log searching in runfo is not very easy for this type of failures and Kusto ingestion happens from AzDO afaik so we can't use it. Is there any tracking issue for this?
cc: @aik-jahoda @ericstj @dotnet/dnceng @dotnet/runtime-infrastructure

safern · 2021-05-12T23:28:43Z

@hoyosjs this is the issue tracking that failure: https://github.com/dotnet/core-eng/issues/13026 and they are working on adding retries to the reporter: dotnet/arcade#7371 so maybe consider pinging there to raise their attention that we are being badly impacted?

MattGal · 2021-05-12T23:29:12Z

Checked on the failures. There's no failed tests. @sandreenko, if you feel like this is ready to merge please do so.

@MattGal I've seen this issue with 503s starting to cause considerable pain, where we'll fail due to reporting even though all tests passed. Helix log searching in runfo is not very easy for this type of failures and Kusto ingestion happens from AzDO afaik so we can't use it. Is there any tracking issue for this?
cc: @aik-jahoda @ericstj @dotnet/dnceng @dotnet/runtime-infrastructure

@hoyosjs nothing's changed about how often the 503s occur, they just now actually report failure starting 4/29. It's a long story, hit me up on Teams for a longer discussion. (Before 4/29, you were just able to merge failing tests if reporting to Azure DevOps failed). There is a simple and very quick fix that also causes failed work items to be logged in the test tab that is undesirable for this reason.

dotnet/arcade#7371 is the FR issue that tracks introducing more retries to the script in the case of Azure Devops failure.

hoyosjs · 2021-05-12T23:31:33Z

Wasn't aware they were getting dropped on the floor. AS always, thanks @MattGal :) And thanks @safern.

JulieLeeMSFT · 2021-05-13T04:46:35Z

Thanks @sandreenko for merging it.

WhiteBlackGoose added 2 commits May 11, 2021 10:06

Replaces all BOOL to bool in RyuJIT

3c6bee8

Replaced all Windows' BOOL to bool in RyuJIT except for WIN API related cases. Fix dotnet#48317

Replaced TRUE and FALSE constants

7424de9

Replaced `TRUE` with `true` and `FALSE` with `false` constants in RyuJIT except Windows-specific cases.

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 11, 2021

WhiteBlackGoose added 2 commits May 11, 2021 11:12

A couple of BOOLs replaced to bool

b512a35

Format issue fixed

0f47310

Formatting patch applied according to the instruction.

WhiteBlackGoose commented May 11, 2021

View reviewed changes

WhiteBlackGoose marked this pull request as ready for review May 11, 2021 17:06

AraHaan approved these changes May 12, 2021

View reviewed changes

JulieLeeMSFT requested a review from sandreenko May 12, 2021 21:33

JulieLeeMSFT assigned WhiteBlackGoose May 12, 2021

JulieLeeMSFT added this to the 6.0.0 milestone May 12, 2021

sandreenko approved these changes May 12, 2021

View reviewed changes

sandreenko merged commit f7805ce into dotnet:main May 12, 2021

ghost locked as resolved and limited conversation to collaborators Jun 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replacing `BOOL` with `bool` #52585

Replacing `BOOL` with `bool` #52585

WhiteBlackGoose commented May 11, 2021

WhiteBlackGoose May 11, 2021

sandreenko May 12, 2021

WhiteBlackGoose May 12, 2021

WhiteBlackGoose commented May 11, 2021

hoyosjs commented May 12, 2021

azure-pipelines bot commented May 12, 2021

hoyosjs commented May 12, 2021

AraHaan left a comment •

edited

Loading

WhiteBlackGoose commented May 12, 2021

JulieLeeMSFT commented May 12, 2021

JulieLeeMSFT commented May 12, 2021

sandreenko commented May 12, 2021

sandreenko left a comment

sandreenko May 12, 2021

SingleAccretion commented May 12, 2021

AraHaan commented May 12, 2021

hoyosjs commented May 12, 2021

safern commented May 12, 2021

MattGal commented May 12, 2021 •

edited

Loading

hoyosjs commented May 12, 2021

JulieLeeMSFT commented May 13, 2021

Replacing BOOL with bool #52585

Replacing BOOL with bool #52585

Conversation

WhiteBlackGoose commented May 11, 2021

WhiteBlackGoose May 11, 2021

Choose a reason for hiding this comment

sandreenko May 12, 2021

Choose a reason for hiding this comment

WhiteBlackGoose May 12, 2021

Choose a reason for hiding this comment

WhiteBlackGoose commented May 11, 2021

hoyosjs commented May 12, 2021

azure-pipelines bot commented May 12, 2021

hoyosjs commented May 12, 2021

AraHaan left a comment • edited Loading

Choose a reason for hiding this comment

WhiteBlackGoose commented May 12, 2021

JulieLeeMSFT commented May 12, 2021

JulieLeeMSFT commented May 12, 2021

sandreenko commented May 12, 2021

sandreenko left a comment

Choose a reason for hiding this comment

sandreenko May 12, 2021

Choose a reason for hiding this comment

SingleAccretion commented May 12, 2021

AraHaan commented May 12, 2021

hoyosjs commented May 12, 2021

safern commented May 12, 2021

MattGal commented May 12, 2021 • edited Loading

hoyosjs commented May 12, 2021

JulieLeeMSFT commented May 13, 2021

Replacing `BOOL` with `bool` #52585

Replacing `BOOL` with `bool` #52585

AraHaan left a comment •

edited

Loading

MattGal commented May 12, 2021 •

edited

Loading