Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

[release/3.1] Fix building with clang 13 #43104

Closed
wants to merge 1 commit into from

Conversation

omajid
Copy link
Member

@omajid omajid commented Sep 1, 2021

Clang now enables cast-function-type warning which results in build
errors that look like this:

/corefx/src/Native/Unix/System.Native/pal_process.c(382,76): error G39B05358: cast from 'void (*)(int, siginfo_t *, void *)' to 'void (*)(int)' converts to incompatible function type [-Werror,-Wcast-function-type] [/corefx/src/Native/build-native.proj]
void (*oldhandler)(int) = (sa_old.sa_flags & SA_SIGINFO) ? (void (*)(int))sa_old.sa_sigaction : sa_old.sa_handler;
                                                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Disable this warning using -Wno-cast-function-type warning. That seems
lower-risk than any functional code change.

Clang 13 also enables reserved-identifier warning which results in build errors
like this:

/corefx/src/Native/Unix/System.Native/pal_process.c(819,17): error GA61F72A6: identifier '__cpu' is reserved because it starts with '__' [-Werror,-Wreserved-identifier] [/corefx/src/Native/build-native.proj]
                  if (CPU_ISSET(cpu, &set))
                      ^
/usr/include/sched.h:94:34: note: expanded from macro 'CPU_ISSET'
# define CPU_ISSET(cpu, cpusetp) __CPU_ISSET_S (cpu, sizeof (cpu_set_t), \
                                 ^

This really seems like a bug in clang that it's complaiing about code in
system libraries that we are using via wrapper-macros. But there are
other instances of this warning that are speciifc to our code too, such
as __padding in
src/Native/Unix/System.Native/pal_interfaceaddresses.h. Disable this
warning too.

Tested with Clang 13 rc1

@omajid omajid force-pushed the 3.1-fix-warnings-clang-13 branch 4 times, most recently from 87b5027 to 9a2586c Compare September 7, 2021 16:03
@safern
Copy link
Member

safern commented Jan 11, 2022

@omajid what's the status of this? Is this still needed?

@omajid
Copy link
Member Author

omajid commented Jan 20, 2022

Yes, still needed: we would like to build .NET Core 3.1 on recent versions of Fedora (and even Alpine, see #43130) that fail because of warnings.

(The alternative is each Linux distro carrying the patches locally, but that seems unnecessary for well understood changes like disabling compiler warnings).

@safern
Copy link
Member

safern commented Jan 20, 2022

Ok, would you mind rebasing on top of the target branch and pushing that?

@omajid
Copy link
Member Author

omajid commented Jan 20, 2022

Yup, just wanted to find some time to see if this improves things on alpine too.

@omajid omajid force-pushed the 3.1-fix-warnings-clang-13 branch 2 times, most recently from 72f0d9a to 124bc55 Compare January 20, 2022 17:42
@omajid
Copy link
Member Author

omajid commented Jan 20, 2022

@safern I have rebased it and re-tested it.

@omajid
Copy link
Member Author

omajid commented Jan 20, 2022

Test failures are in libraries, which look unrelated to my changes?

@danmoseley
Copy link
Member

The failures are unrelated but would be fixed by grabbing these lines. Could go in this PR or another.
https://github.com/dotnet/runtime/pull/62559/files#diff-18bd39fa893065cf18d4932a2a06e9796381e5548a5a809d3fd53c83b3bdb14dR131-R144

@safern
Copy link
Member

safern commented Jan 21, 2022

Branch is closed as of now. But we can merge this when is open.

Thanks, @danmoseley for the pointer. If @omajid has time to port that it can be done here, if not I'll do it in a separate PR.

@carlossanlop
Copy link
Member

@omajid the branch is open again. Can you please port the suggested lines mentioned above to get rid of the failure?

@omajid omajid force-pushed the 3.1-fix-warnings-clang-13 branch from 124bc55 to cb14d25 Compare May 26, 2022 21:54
@omajid
Copy link
Member Author

omajid commented Jun 1, 2022

The test fixes were ported separately by #43137.

I have rebased this PR on top of that.

Clang 13 now enables cast-function-type warning which results in build
errors that look like this:

    /corefx/src/Native/Unix/System.Native/pal_process.c(382,76): error G39B05358: cast from 'void (*)(int, siginfo_t *, void *)' to 'void (*)(int)' converts to incompatible function type [-Werror,-Wcast-function-type] [/corefx/src/Native/build-native.proj]
    void (*oldhandler)(int) = (sa_old.sa_flags & SA_SIGINFO) ? (void (*)(int))sa_old.sa_sigaction : sa_old.sa_handler;
                                                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Disable this warning using -Wno-cast-function-type warning. That seems
lower-risk than any functional code change.

Clang 13 also enables reserved-identifier warning which results in build errors
like this:

    /corefx/src/Native/Unix/System.Native/pal_process.c(819,17): error GA61F72A6: identifier '__cpu' is reserved because it starts with '__' [-Werror,-Wreserved-identifier] [/corefx/src/Native/build-native.proj]
                      if (CPU_ISSET(cpu, &set))
                          ^
    /usr/include/sched.h:94:34: note: expanded from macro 'CPU_ISSET'
    # define CPU_ISSET(cpu, cpusetp) __CPU_ISSET_S (cpu, sizeof (cpu_set_t), \
                                     ^

This really seems like a bug in clang that it's complaining about code in
system libraries that we are using via wrapper-macros. But there are
other instances of this warning that are speciifc to our code too, such
as `__padding` in
src/Native/Unix/System.Native/pal_interfaceaddresses.h. Disable this
warning too.

Tested with Clang 13 rc1.
@omajid omajid force-pushed the 3.1-fix-warnings-clang-13 branch from cb14d25 to 8cfbb7e Compare June 28, 2022 21:55
@carlossanlop
Copy link
Member

@hoyosjs @jkotas need to consult with you: do you have any objections on taking this 3.1 change? And do cmake changes need to go through Tactics, or can we just consider them infra [tell-mode]?

@jkotas
Copy link
Member

jkotas commented Sep 7, 2022

Have you looked at the CI failures and determined that they are unrelated?

This is same type of change as #42900. It is fine to take it. It should follow the same process as #42900 .

@hoyosjs
Copy link
Member

hoyosjs commented Sep 7, 2022

/azp run corefx-ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@hoyosjs hoyosjs left a comment

Choose a reason for hiding this comment

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

The changes themselves make sense and are the least risky for servicing. @danmoseley can this be servicing-consider?

@danmoseley
Copy link
Member

Is it worth it? It is out of support in 4 months.

@carlossanlop
Copy link
Member

Closing since 3.1 is going out of support.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants