Skip to content

Conversation

@AaronRobinsonMSFT
Copy link
Member

Checking #82052 regression.

Reverts #81604

@ghost ghost added the area-PAL-coreclr label Feb 13, 2023
@AaronRobinsonMSFT
Copy link
Member Author

/azp list

@azure-pipelines
Copy link

CI/CD Pipelines for this repository:

@AaronRobinsonMSFT
Copy link
Member Author

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AaronRobinsonMSFT
Copy link
Member Author

/cc @trylek

@trylek trylek requested a review from janvorli February 13, 2023 21:55
@AaronRobinsonMSFT
Copy link
Member Author

@am11 All legs appear to be passing here after being reverted. Am I missing something?

@am11
Copy link
Member

am11 commented Feb 14, 2023

Am I missing something?

One of two reasons: 1) CI doesn't run PAL tests on osx-arm64 or 2) CI is using Xcode version < 14.2.

@AaronRobinsonMSFT
Copy link
Member Author

Am I missing something?

One of two reasons: 1) CI doesn't run PAL tests on osx-arm64 or 2) CI is using Xcode version < 14.2.

Okay. I run an osx-arm64. I will give it a shot.

@am11
Copy link
Member

am11 commented Feb 14, 2023

The error #81604 (comment) which I was getting while building clr.paltests is the same which we saw earlier during src/tests build #77950 (comment).

This nopal test is basically testing system implementation of sprintf which we have avoided in places where PAL is not available. It was strange that switching to snprintf breaks NamedMutex tests on ARM. Maybe it's better to just delete test_sprintf, since we are not using this API from the system any longer? (we are using the one from PAL in a few places)

@janvorli
Copy link
Member

It was strange that switching to snprintf breaks NamedMutex tests on ARM

@am11 it seems it actually fails on all architectures, see #82068

@am11
Copy link
Member

am11 commented Feb 14, 2023

@janvorli yup, I commented here yesterday: #82052 (comment) it's failing on arm{64} and linux-musl (all arch). So yes, it is not failing "everywhere". e.g. linux-x64 is green, but linux-musl-x64 isn't in this run: https://dev.azure.com/dnceng-public/public/_build/results?buildId=169947&view=results (both pipelines executed PAL tests)

@janvorli
Copy link
Member

@am11 I am looking into why it fails.

@janvorli
Copy link
Member

I have tried Ubuntu arm64 debug / checked and x64 Alpine debug/checked. I cannot repro the issue (neither the test failure nor the

paltests <PrintPalTests|TestName>
Either print list of all paltests by passing PrintPalTests, or run a single PAL test.

@janvorli
Copy link
Member

Neither Release build reproes that for me. For Alpine, I was using the very same container that the CI uses. So I wonder if the fact that I am using the runpaltests.sh while the CI uses the runpaltestshelix.sh can have any effect. Trying to run that script now.

@janvorli
Copy link
Member

Ok, so downloading the build artifacts from CI and running the test with them finally reproduced the problem (both the strange message and the error). I keep investigating.

@AaronRobinsonMSFT
Copy link
Member Author

Maybe it's better to just delete test_sprintf, since we are not using this API from the system any longer? (we are using the one from PAL in a few places)

I would agree with this sentiment. The only scenario I think we need to continue to validate for printf APIs are those involving the "secure" (that is, _s suffix) APIs that perform truncation.

@janvorli
Copy link
Member

janvorli commented Feb 14, 2023

This is wrong:

   196      processCommandLinePathLength += test_snprintf(&g_processCommandLinePath[processCommandLinePathLength], processCommandLinePathLength, "%s ", "threading/NamedMutex/test1/paltest_namedmutex_test1");

The length passed to that is not correct, it should be 4096 - processCommandLinePathLength. I'm not sure though how that would cause the failure.

The funny thing is that the reason why it failed only for the artifacts from the CI and not my local build from the same checkin on the same platform is the path length. I've put the CI artifacts to /home/helixbot/test while my build had them in /home/helixbot/runtime/artifacts/bin/coreclr/linux.x64.Debug/. When I copied my build artifacts to /home/helixbot/test, the issue started to repro too.

@am11
Copy link
Member

am11 commented Feb 14, 2023

@janvorli, thank you for looking into it. I was able to repro it with your steps. Could you please take a look at the fix #82122? I have used precise measurements.

@AaronRobinsonMSFT
Copy link
Member Author

Deferring to proposed fix in #82122

@AaronRobinsonMSFT AaronRobinsonMSFT deleted the revert-81604-feature/pal-tests branch February 14, 2023 19:35
@ghost ghost locked as resolved and limited conversation to collaborators Mar 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants