Fix gcc armel/arm64 build #53220

gbalykov · 2021-05-25T09:43:35Z

Mostly signed/unsigned comparisons, etc.

sandreenko · 2021-05-26T03:06:04Z

src/coreclr/jit/codegenarm.cpp

@@ -460,7 +460,7 @@ void CodeGen::genLclHeap(GenTree* tree)
        }

        // regCnt will be the total number of bytes to locAlloc
-        genSetRegToIcon(regCnt, amount, ((int)amount == amount) ? TYP_INT : TYP_LONG);
+        genSetRegToIcon(regCnt, amount, ((int)amount >= 0) ? TYP_INT : TYP_LONG);


this does not look right, a narrowing cast is implementation-defined in C++.
also, it looks like we don't support TYP_LONG in

runtime/src/coreclr/jit/codegenarm.cpp

Line 183 in eee0ed2

// TODO-CrossBitness: we wouldn't need the cast below if we had CodeGen::instGen_Set_Reg_To_Reloc_Imm.

that we are calling here, maybe use ClrSafeInt and cast to int always? cc @echesakovMSFT

Yes, my bad, thanks. But it seems that original code is incorrect too

I don't think this is relevant to CodeGen::instGen_Set_Reg_To_Reloc_Imm issue in cross-bitness scenarious.

The change is for localloc (i.e. .NET way of alloca).

According to ECMA-335

The localloc instruction allocates size (type native unsigned int or U4) bytes from the local
dynamic memory pool ...

I agree with Sergey that we should always use TYP_INT here.

Suggested change

genSetRegToIcon(regCnt, amount, ((int)amount >= 0) ? TYP_INT : TYP_LONG);

genSetRegToIcon(regCnt, amount, TYP_INT);

Fixed, thanks

sandreenko · 2021-05-26T03:07:36Z

src/coreclr/pal/src/libunwind/src/arm/Gtrace.c

@@ -526,7 +526,7 @@ tdep_trace (unw_cursor_t *cursor, void **buffer, int *size)
      break;

    case UNW_ARM_FRAME_SYSCALL:
-      printf("XXX1\n");
+      //printf("XXX1\n");


not familiar with this code, what was the problem?

[ 71s] In file included from /home/abuild/rpmbuild/BUILD/coreclr-6.0.0/src/coreclr/pal/src/libunwind/src/arm/Ltrace.c:4: [ 71s] /home/abuild/rpmbuild/BUILD/coreclr-6.0.0/src/coreclr/pal/src/libunwind/src/arm/Gtrace.c: In function '_ULarm_tdep_trace': [ 71s] /home/abuild/rpmbuild/BUILD/coreclr-6.0.0/src/coreclr/pal/src/libunwind/src/arm/Gtrace.c:529:7: error: incompatible implicit declaration of built-in function 'printf' [-Werror] [ 71s] 529 | printf("XXX1\n"); [ 71s] | ^~~~~~ [ 71s] /home/abuild/rpmbuild/BUILD/coreclr-6.0.0/src/coreclr/pal/src/libunwind/src/arm/Gtrace.c:31:1: note: include '<stdio.h>' or provide a declaration of 'printf' [ 71s] 30 | #include <limits.h> [ 71s] +++ |+#include <stdio.h> [ 71s] 31 |

Would adding #include <stdio.h> at the beginning of the file fix the issue or it would introduce some other errors?
cc @janvorli

This should not be modified here. We use a verbatim copy of the libunwind library (https://github.com/libunwind/libunwind) and all necessary changes should be made upstream.

I've removed this change from here

sandreenko · 2021-05-26T03:07:48Z

cc @dotnet/jit-contrib

echesakov · 2021-05-26T16:26:19Z

src/coreclr/ToolBox/superpmi/superpmi-shared/lightweightmap.h

@@ -319,7 +319,7 @@ class LightWeightMap : public LightWeightMapBuffer

        // If we have RTTI, we can make this assert report the correct type. No RTTI, though, when
        // built with .NET Core, especially when built against the PAL.
-        AssertCodeMsg((ptr - bytes) == size, EXCEPTIONCODE_LWM, "%s - Ended with unexpected sizes %p != %x",
+        AssertCodeMsg((unsigned int)(ptr - bytes) == size, EXCEPTIONCODE_LWM, "%s - Ended with unexpected sizes %p != %x",


I would rewrite it as follows to avoid casting

Suggested change

AssertCodeMsg((unsigned int)(ptr - bytes) == size, EXCEPTIONCODE_LWM, "%s - Ended with unexpected sizes %p != %x",

AssertCodeMsg(ptr == (bytes + size), EXCEPTIONCODE_LWM, "%s - Ended with unexpected sizes %p != %x",

echesakov · 2021-05-26T16:31:25Z

src/coreclr/ToolBox/superpmi/superpmi-shared/lightweightmap.h

@@ -658,7 +658,7 @@ class DenseLightWeightMap : public LightWeightMapBuffer
            ptr += bufferLength * sizeof(unsigned char);
        }

-        AssertCodeMsg((ptr - bytes) == size, EXCEPTIONCODE_LWM, "Ended with unexpected sizes %Ix != %x", ptr - bytes,
+        AssertCodeMsg((unsigned int)(ptr - bytes) == size, EXCEPTIONCODE_LWM, "Ended with unexpected sizes %Ix != %x", ptr - bytes,


Same here

Suggested change

AssertCodeMsg((unsigned int)(ptr - bytes) == size, EXCEPTIONCODE_LWM, "Ended with unexpected sizes %Ix != %x", ptr - bytes,

AssertCodeMsg(ptr == (bytes + size), EXCEPTIONCODE_LWM, "Ended with unexpected sizes %Ix != %x", ptr - bytes,

echesakov · 2021-05-26T16:46:32Z

src/coreclr/jit/codegenarm.cpp

@@ -460,7 +460,7 @@ void CodeGen::genLclHeap(GenTree* tree)
        }

        // regCnt will be the total number of bytes to locAlloc
-        genSetRegToIcon(regCnt, amount, ((int)amount == amount) ? TYP_INT : TYP_LONG);
+        genSetRegToIcon(regCnt, amount, ((int)amount >= 0) ? TYP_INT : TYP_LONG);


I don't think this is relevant to CodeGen::instGen_Set_Reg_To_Reloc_Imm issue in cross-bitness scenarious.

The change is for localloc (i.e. .NET way of alloca).

According to ECMA-335

The localloc instruction allocates size (type native unsigned int or U4) bytes from the local
dynamic memory pool ...

I agree with Sergey that we should always use TYP_INT here.

Suggested change

genSetRegToIcon(regCnt, amount, ((int)amount >= 0) ? TYP_INT : TYP_LONG);

genSetRegToIcon(regCnt, amount, TYP_INT);

echesakov · 2021-05-26T16:54:22Z

src/coreclr/pal/src/CMakeLists.txt

@@ -112,10 +112,6 @@ if(CLR_CMAKE_HOST_ARCH_ARM)
  endif()
 endif(CLR_CMAKE_HOST_ARCH_ARM)

-if (CMAKE_CXX_COMPILER_ID MATCHES "GNU")


Can you please give an example why the change is needed? Is it assembler related?

This should stay, but the condition should be updated so that it gets added only for Intel processors (CLR_CMAKE_HOST_ARCH_AMD64 or CLR_CMAKE_HOST_ARCH_I386).

arm assembler doesn't have this option, and linux x64 shows:

$ as --help|grep divide --divide ignored

Why is this option needed?

This was added for SunOS support. @am11 do you remember why was it needed?

On SYS-V derivative platforms, / in assembly is treated as # rather than division. In context2.S, we are using division, so it breaks build on platforms, such as SunOS-likes and QNX.

Please note that GNU toolchain support is strictly added for community supported platforms in .NET, where LLVM toolchain is either not available or not as complete. If your platform already has good support for LLVM toolchain, it is recommended (per its official support stauts) to use that instead.

Thanks for explanation. I've kept the original GNU condition and added x86/x64 check.

echesakov · 2021-05-26T16:55:58Z

src/coreclr/pal/src/libunwind/src/arm/Gtrace.c

@@ -526,7 +526,7 @@ tdep_trace (unw_cursor_t *cursor, void **buffer, int *size)
      break;

    case UNW_ARM_FRAME_SYSCALL:
-      printf("XXX1\n");
+      //printf("XXX1\n");


Would adding #include <stdio.h> at the beginning of the file fix the issue or it would introduce some other errors?
cc @janvorli

echesakov · 2021-05-26T16:56:35Z

src/coreclr/pal/src/misc/jitsupport.cpp

@@ -82,7 +82,7 @@ static const CpuCapability CpuCapabilities[] = {
 // If the capability name is not recognized or unused at present, zero is returned.
 static unsigned long LookupCpuCapabilityFlag(const char* start, size_t length)
 {
-    for (int i = 0; i < _countof(CpuCapabilities); i++)
+    for (int i = 0; i < (int)_countof(CpuCapabilities); i++)


I would avoid casting

Suggested change

for (int i = 0; i < (int)_countof(CpuCapabilities); i++)

for (size_t i = 0; i < _countof(CpuCapabilities); i++)

Mostly signed/unsigned comparisons, etc.

sandreenko

I see that the PR was updated, LGTM, thanks for making the changes.

BruceForstall · 2021-06-09T23:28:40Z

Test failures are #53311

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 25, 2021

gbalykov force-pushed the gcc-armel-build branch 2 times, most recently from 7de81d5 to 7ab98a8 Compare May 25, 2021 11:01

gbalykov changed the title ~~Fix gcc armel build~~ Fix gcc armel/arm64 build May 25, 2021

sandreenko reviewed May 26, 2021

View reviewed changes

echesakov reviewed May 26, 2021

View reviewed changes

gbalykov force-pushed the gcc-armel-build branch 2 times, most recently from d05eb6a to 7bd8bb6 Compare May 27, 2021 07:03

gbalykov added 2 commits May 27, 2021 10:07

Fix gcc armel build

161b0da

Mostly signed/unsigned comparisons, etc.

Fix gcc arm64 build

efbd56a

gbalykov force-pushed the gcc-armel-build branch from 7bd8bb6 to efbd56a Compare May 27, 2021 07:08

runfoapp bot mentioned this pull request May 27, 2021

FileSystemWatcher_DirectorySymbolicLink_TargetsFile_Fails #53366

Open

sandreenko approved these changes Jun 6, 2021

View reviewed changes

echesakov approved these changes Jun 7, 2021

View reviewed changes

BruceForstall merged commit f6c52b9 into dotnet:main Jun 9, 2021

ghost locked as resolved and limited conversation to collaborators Jul 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix gcc armel/arm64 build #53220

Fix gcc armel/arm64 build #53220

gbalykov commented May 25, 2021 •

edited

Loading

sandreenko May 26, 2021

gbalykov May 26, 2021

echesakov May 26, 2021

gbalykov May 27, 2021

sandreenko May 26, 2021

gbalykov May 26, 2021

echesakov May 26, 2021

janvorli May 26, 2021

gbalykov May 27, 2021

sandreenko commented May 26, 2021

echesakov May 26, 2021

echesakov May 26, 2021

echesakov May 26, 2021

echesakov May 26, 2021

janvorli May 26, 2021

gbalykov May 26, 2021

janvorli May 26, 2021

am11 May 27, 2021

gbalykov May 27, 2021

echesakov May 26, 2021

echesakov May 26, 2021

sandreenko left a comment

BruceForstall commented Jun 9, 2021

	genSetRegToIcon(regCnt, amount, ((int)amount >= 0) ? TYP_INT : TYP_LONG);
	genSetRegToIcon(regCnt, amount, TYP_INT);

	AssertCodeMsg((unsigned int)(ptr - bytes) == size, EXCEPTIONCODE_LWM, "%s - Ended with unexpected sizes %p != %x",
	AssertCodeMsg(ptr == (bytes + size), EXCEPTIONCODE_LWM, "%s - Ended with unexpected sizes %p != %x",

	for (int i = 0; i < (int)_countof(CpuCapabilities); i++)
	for (size_t i = 0; i < _countof(CpuCapabilities); i++)

Fix gcc armel/arm64 build #53220

Fix gcc armel/arm64 build #53220

Conversation

gbalykov commented May 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandreenko commented May 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandreenko left a comment

Choose a reason for hiding this comment

BruceForstall commented Jun 9, 2021

gbalykov commented May 25, 2021 •

edited

Loading