-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Conversation
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM.
- @lateralusX should take a look at the EP changes
- I don't understand the change from
MAX_SYMBOL_SIZE
to 1024 inemit_info_symbol
- 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
fe88ad5
to
dedf251
Compare
@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: cc @akoeplinger |
There was a problem hiding this 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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok sounds good then :)
23e27ff
to
9fdc62f
Compare
9fdc62f
to
1962f6f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome ❤️
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:
mcr.microsoft.com/dotnet-buildtools/prereqs:debian-11-gcc11-amd64-20210916125744-f42d766
runtime/build.sh mono -gcc -c Debug
(and later with-c Release
)