Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a few gcc warnings in jit #57744

Merged
merged 1 commit into from
Aug 22, 2021
Merged

Conversation

am11
Copy link
Member

@am11 am11 commented Aug 19, 2021

Fixes #57861

Cleans up 1045 occurrences of warning from gcc debug build logs. Remaining one compiler warning related to 'bit field width overflow', which has 2565 occurrences, is tracked by #33541.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 19, 2021
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 19, 2021
@ghost
Copy link

ghost commented Aug 19, 2021

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

Issue Details

Cleans up 1045 occurrences of warning from gcc debug build logs. Remaining one compiler warning related to 'bit field width overflow', which has 2565 occurrences, is tracked by #33541.

Author: am11
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@am11
Copy link
Member Author

am11 commented Aug 19, 2021

Some more details..
The warn-as-error -Wall compiler flag is enabled for all compilers, for all non-mono subsets. The exception is jit with gcc, where -Wall is explicitly disabled due to log flood caused by bit field width overflow issue (see #33541 (comment)). Aside from that issue, there were a couple of warnings found in the build logs by applying the following filter, that this PR aims to fix:
./build.sh clr -gcc 2>&1 | grep -i warn | sort -u | grep -v "too small to hold all values"

There is one occurrence of a warning from gas assembler 'Warning: ignoring changed section attributes for .rodata' and that is not related to gs_cookie (instead it's some intermediate assembly generated in /tmp; I'll investigate if there is anything actionable). All remaining 2564 warnings are in jit related to the same bit field width.

@SingleAccretion
Copy link
Contributor

SingleAccretion commented Aug 19, 2021

I think the proper way to fix the range checks is to use the FitsIn<T>() method (e. g. FitsIn<int32_t>(value)).

@am11
Copy link
Member Author

am11 commented Aug 19, 2021

I think the proper way to fix the range checks is to use the FitsIn<T>() method (e. g. FitsIn<int32_t>(value)).

Thanks, wow didn't realize something like that exists. I will try FitsIn<T> next. 👍
I was looking a way to plumb std::numeric_limits<T>::max and friends through PAL, experimented with it a bit, but unsurprisingly it had longer tail (works on one platform doesn't meant it'd cover the matrix). 😅

@am11 am11 force-pushed the fix/jit-warnings branch from 388d2bc to d146a37 Compare August 19, 2021 21:03
@am11 am11 force-pushed the fix/jit-warnings branch from 8c6d68c to 7c86efa Compare August 21, 2021 16:58
@am11
Copy link
Member Author

am11 commented Aug 21, 2021

@jakobbotsch, should we worry about other usages of FitsIn<T>() on 32-bit platforms and fix that for cases like FitsIn<int>(anUnsignedInt)? Note that we have globally turned off family of tautological comparison warnings, so bug in utility function can get slipped rather easily.

@jakobbotsch
Copy link
Member

@jakobbotsch, should we worry about other usages of FitsIn() on 32-bit platforms and fix that for cases like FitsIn(anUnsignedInt)?

I don't think so, FitsIn is a quite semantic operation, asking if the "semantic" value can be represented in another data type. It's not always the operation we want in the JIT when we want to cast between signed/unsigned values for various reasons.

@jakobbotsch jakobbotsch merged commit 69bdfaa into dotnet:main Aug 22, 2021
@jakobbotsch
Copy link
Member

Thanks!

@am11 am11 deleted the fix/jit-warnings branch August 22, 2021 13:42
@ghost ghost locked as resolved and limited conversation to collaborators Sep 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GCC build failure for JIT code
3 participants