corefx [release/3.1] Fix building with clang 13

[release/3.1] Fix building with clang 13

Open omajid opened this issue 2 years ago • 15 comments

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

Sep 01 '21 23:09 omajid

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

Jan 11 '22 18:01 safern

Yes, still needed: we would like to build .NET Core 3.1 on recent versions of Fedora (and even Alpine, see https://github.com/dotnet/corefx/pull/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).

Jan 20 '22 00:01 omajid

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

Jan 20 '22 00:01 safern

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

Jan 20 '22 01:01 omajid

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

Jan 20 '22 17:01 omajid

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

Jan 20 '22 23:01 omajid

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

Jan 21 '22 00:01 danmoseley

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.

Jan 21 '22 01:01 safern

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

Apr 05 '22 21:04 carlossanlop

The test fixes were ported separately by https://github.com/dotnet/corefx/pull/43137.

I have rebased this PR on top of that.

Jun 01 '22 20:06 omajid

@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]?

Sep 07 '22 20:09 carlossanlop

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

This is same type of change as https://github.com/dotnet/corefx/pull/42900. It is fine to take it. It should follow the same process as #42900 .