Remove LittleEndian asserts #75159
Remove LittleEndian asserts #75159
Conversation
The functionality work both on little endian and bigendian architecture. Hence removing the assert statements. These can be verified by running CommandLine test suite.
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
dotnet-policy-service
bot
added
the
Community
|
I am not sure if these failures are because of my commit. As I can see from below |
|
I am curious what prompted this change. Are you running into the asserts in some scenarios? Could you provide details? |
|
Well, if "Command line" test suits from Csharp/Test/ are run on S390x architecture , these asserts are triggered. |
From a quick conversation with @VSadov, we probably do some bit/byte manipulations that we are not sure are correct on a big-endian machine and could never test that. Unfortunately, CommandLine tests is a very limited set of tests that do not really provide coverage for full breadth of the scenarios. If, let's say, all tests were passing, that would add more confidence. In addition, we think it would be good to explicitly go over the code in the affected components to confirm it is correct for big-endian platforms. There is no confidence that we have existing unit-tests covering the "interesting" scenarios. |
|
The command Line test cases were failing on big-endian architectures. Upon investigating the cause of the problem, got to know that asserts were triggered. After removing these asserts, the test cases started passing. Is there anything needed to be verified on big endian architecture to ensure that these test cases are not only meant for little endian machines ? |
|
cc @uweigand |
As a starting point I would remove those asserts locally and re-run the entire test suite, not just CommandLineTests, and see if that revealed any issues. The tests passing here doesn't mean much with the attribute removed. Overall though I think it would help if you gave some context here. There are now a few PRs out where it appears you're attempting to get Roslyn to run in a new environment. Could you tell us a bit more about what you're doing here? Maybe open an issue / discussion where we can see what the bigger goals are. Otherwise this just seems like one off PRs that look a bit off to us and without context we're not sure what to do with them. |
Hi @jaredpar , I understand @giritrivedi is out this week, so let me try to answer this. This effort is not about supporting any new platforms as such, but it is about the IBM community-supported platforms IBM Z (s390x) and POWER (ppc64le). We already support .NET using the Mono runtime on these platforms since .NET 6 (or 7), and roslyn in general is working fine - e.g. we can use roslyn natively on these platforms to build itself, or even build the full source-build .NET. Also, most of the roslyn test suite is passing - any major issue has been fixed back in the .NET 6 days. However, we never got around to making the roslyn test suite fully green on our platforms. To ensure ongoing support going forward, we're now trying to fix that - get to a state where the test suite passes successfully 100%, and then set up an ongoing CI to keep it that way. Note that the vast majority of tests already pass, but there's a small number (around 50 out of the 30000 tests) that fail due to various issues, which look to be either test suite problems (incompatibility with big-endian and/or the Mono runtime), or some minor differences in behavior between the Mono and CoreCLR runtime that doesn't cause any problems for actual code, but are visible to some of the low-level roslyn tests. What I've asked @giritrivedi to do is to go through that set of remaining test case failures and address them, typically by fixing some of those incompatibilities where feasible, or else disabling the tests if appropriate. (Anything that should turn out to be some actual problem we had overlooked so far would of course also have to be addressed.) |
|
@jaredpar @AlekseyTs for second review. Thanks |
|
Merged/squashed. Thanks @giritrivedi for the contribution |
The functionality work both on little endian and bigendian architecture. Hence removing the assert statements. These can be verified by running CommandLine test suite.