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 gcc warnings during mono linux-x64 build #60675

Merged
merged 4 commits into from
Nov 2, 2021

Conversation

am11
Copy link
Member

@am11 am11 commented Oct 20, 2021

main with Debug configuration: 822 warnings - http://sprunge.us/2GzrDE
PR with Debug configuration: 3 warnings related to deprecated sys/sysctl.h includes - http://sprunge.us/JuyA3K

after fixing Debug warnings, there were 13 additional warnings in Release configuration:
http://sprunge.us/PJCivP

PR with Release configuration: (same) 3 warnings - http://sprunge.us/NwKHNE

Used:

  • same docker image used in CI for coreclr gcc build: mcr.microsoft.com/dotnet-buildtools/prereqs:debian-11-gcc11-amd64-20210916125744-f42d766
  • build command: runtime/build.sh mono -gcc -c Debug (and later with -c Release)

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 20, 2021
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM.

  1. @lateralusX should take a look at the EP changes
  2. I don't understand the change from MAX_SYMBOL_SIZE to 1024 in emit_info_symbol
  3. For the components it would be better to rename the global symbols with prefixes&suffixes and add a comment to not use them directly.

main with Debug configuration: 822 warnings - http://sprunge.us/2GzrDE
PR with Debug configuration: 3 warnings related to deprecated sys/sysctl.h includes - http://sprunge.us/JuyA3K

after fixing Debug warnings, there were 13 additional warnings in Release configuration:
http://sprunge.us/PJCivP

PR with Release configuration: (same) 3 warnings - http://sprunge.us/NwKHNE
@am11 am11 force-pushed the feature/native/gcc/mono branch from fe88ad5 to dedf251 Compare October 25, 2021 02:00
@am11 am11 marked this pull request as ready for review October 25, 2021 04:03
@am11
Copy link
Member Author

am11 commented Oct 25, 2021

@lambdageek, I have reverted files which are pulled in by #60725 and #60726.

It would be great to have a gcc leg for mono in CI as well to preserve the build status from regressing. Separate leg or as a step to the existing:
2021-10-25_573x417_scrot
(mono build with gcc; ./build.sh mono -gcc takes < 2 mins..)

cc @akoeplinger

Copy link
Member

@akoeplinger akoeplinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, found one thing to address and one question but looks good otherwise :)

/*
* Suspend creation of new threadpool threads, since they cannot run
*/
tp_suspend = TRUE;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is no longer used because this code from mono/mono was undefined in ENABLE_NETCORE and removed in dotnet/runtime: https://github.com/mono/mono/blob/8b3b6666a858e02137b192c923727bcfd00ef620/mono/mini/debugger-agent.c#L2873-L2876

@lambdageek @vargaz do you know if that is intentional, i.e. do we no longer need to suspend creating threadpool threads? or is this just something that was missed while porting to dotnet/runtime now that the threadpool works differently?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe @vargaz remembers. I think we don't need it anymore - the old threadpool had a native Monitor thread that was responsible for monitoring workers. So the debugger stopped all the managed worker threads, but it didn't have any idea about the monitor thread. In netcore, we don't have a native monitor thread - everything is done by managed threads. So when the debugger suspends the runtime, it will suspend the thread pool worker creation, too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok sounds good then :)

@am11 am11 force-pushed the feature/native/gcc/mono branch 3 times, most recently from 23e27ff to 9fdc62f Compare October 30, 2021 01:53
@am11 am11 force-pushed the feature/native/gcc/mono branch from 9fdc62f to 1962f6f Compare October 31, 2021 14:28
Copy link
Member

@akoeplinger akoeplinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome ❤️

@akoeplinger akoeplinger merged commit 577a70a into dotnet:main Nov 2, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Dec 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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.

5 participants