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 src/libraries to build on clang 10 #33734

Merged
merged 1 commit into from
Mar 20, 2020
Merged

Conversation

omajid
Copy link
Member

@omajid omajid commented Mar 18, 2020

Clang 10 adds/enables new warnings, some of which is affecting thesrc/libraries code.

Clang 10 has added -Walloca to warn about uses of alloca. This commit replaces the only non-compliant use of that with a single fixed stack-allocated buffer.

Clang 10 has also added -Wimplicit-int-float-conversion. This commit uses explicit casts to double to avoid the warnings.

Fixes #33681

Also contains a small fix for slist.h that was somehow missed in #33096.

After this commit, I can build all of runtime with Clang 10.

@omajid
Copy link
Member Author

omajid commented Mar 18, 2020

cc @jkotas

@@ -921,7 +921,16 @@ int32_t SystemNative_Poll(PollEvent* pollEvents, uint32_t eventCount, int32_t mi


int useStackBuffer = bufferSize <= 2048;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
int useStackBuffer = bufferSize <= 2048;
int useStackBuffer = bufferSize <= sizeof(stackBuffer);

... and move it after stackBuffer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Done.

@omajid omajid force-pushed the libraries-clang10 branch from b8d12fd to 7223624 Compare March 18, 2020 21:01
@@ -920,8 +920,17 @@ int32_t SystemNative_Poll(PollEvent* pollEvents, uint32_t eventCount, int32_t mi
}


int useStackBuffer = bufferSize <= 2048;
struct pollfd* pollfds = (struct pollfd*)(useStackBuffer ? alloca(bufferSize) : malloc(bufferSize));
char stackBuffer[2048];
Copy link
Contributor

@lpereira lpereira Mar 18, 2020

Choose a reason for hiding this comment

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

Why not allocate a struct pollfd directly instead of doing these casts? Might even defer the multiply-with-overflow-check to a call to calloc(). If SystemNative_Poll() is mostly moved to an auxiliary function, the stack doesn't even need to be moved to allocate the 2048 bytes every time if the chunk of memory is allocated in the heap:

int32_t SystemNative_Poll(PollEvent* pollEvents, uint32_t eventCount, int32_t milliseconds, uint32_t* triggered)
{
    static const uint32_t mallocThreshold = (uint32_t)(2048 / sizeof(struct pollfd));

    if (pollEvents == NULL || triggered == NULL)
    {
        return Error_EFAULT;
    }
    if (milliseconds < -1)
    {
        return Error_EINVAL;
    }

    if (eventCount > mallocThreshold)
    {
                struct pollfd *pollfds = calloc(eventCount, sizeof(*pollfds));

                if (!pollfds) return Error_ENOMEM;

                int32_t ret = PollInternal(pollEvents, pollfds, eventCount, milliseconds, triggered);
                free(pollfds);
                return ret;
    }

    struct pollfds pollfds[mallocThreshold];
    return PollInternal(pollEvents, pollfds, eventCount, milliseconds, triggered);
}

This also removes some of the branches in the actual function doing the work (e.g. all the "are we using malloc or allocating on the stack" kind of deals.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! However, this leads to:

/home/omajid/devel/dotnet/runtime/src/libraries/Native/Unix/System.Native/pal_io.c(931,27): error GEF718E4C: variable length array folded to constant array as an extension [-Werror,-Wgnu-folding-constant] [/home/omajid/devel/dotnet/runtime/src/libraries/Native/build-native.proj]                                                                                                                                                                                                     
  struct pollfd pollfds[mallocThreshold];           
                        ^~~~~~~~~~~~~~~ 

Should I disable this warning?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, that's pseudo-code. You can probably use a #define here to work around the warning instead of disabling the warning.

Copy link
Member

Choose a reason for hiding this comment

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

the stack doesn't even need to be moved to allocate the 2048 bytes every time if the chunk of memory is allocated in the heap

It is not how to C compilers work. It does not matter that you have the variable in the middle of the method. The stack for it will get allocate in the method prolog regardless.

This also removes some of the branches in the actual function doing the work

It replaces the branching with a call that is likely going to be more expensive.

If you have split the function into two to make the code style better (personally, I liked the single method better), then it is ok. But if you have split the function into two to make it more efficient, I believe that it is doing exact opposite.

Copy link
Member Author

Choose a reason for hiding this comment

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

It sounds like I should revert my patch to be the original/more-minimal version? And any correctness, style or performance issues that also affected the original code should be handled separately?

Copy link
Member

Choose a reason for hiding this comment

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

Replacing malloc with calloc is good, but I would undo the split into two methods.

else
{
pollfds = (struct pollfd*)(malloc(bufferSize));
}
if (pollfds == NULL)
{
return Error_ENOMEM;
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, unrelated to this commit: the events in poll() is a bitmask. The conversion to/from the PAL seems wrong (it's probably fine if nobody ever asks for something like "I want Input and Priority events".)

@omajid omajid force-pushed the libraries-clang10 branch 3 times, most recently from 06075ca to 69ceb82 Compare March 19, 2020 19:19
Clang 10 enable new warnings, some of which is affecting the
src/libraries code.

Clang 10 has added `-Walloca` to warn about uses of `alloca`. This
commit replaces the only non-compliant use of that with a single fixed
stack-allocated buffer.

Clang 10 has also added `-Wimplicit-int-float-conversion`. This commit
uses explicit casts to double to avoid the warnings.

Fixes dotnet#33681

Also contains a small fix for slist.h that was somehow missed in dotnet#33096.

After this commit, I can build all of runtime with Clang 10.
@omajid omajid force-pushed the libraries-clang10 branch from 69ceb82 to 00a63bb Compare March 19, 2020 21:37
@jkotas jkotas merged commit 2cf9265 into dotnet:master Mar 20, 2020
omajid added a commit to omajid/dotnet-corefx that referenced this pull request Apr 3, 2020
Clang 10 adds/enables new warnings, some of which is affecting
the corefx code.

Clang 10 has added -Walloca to warn about uses of alloca. This commit
replaces the only non-compliant use of that with a single fixed
stack-allocated buffer.

Clang 10 has also added -Wimplicit-int-float-conversion. This commit
uses explicit casts to double to avoid the warnings.

This is a backport of dotnet/runtime#33734 to corefx.

After this commit, I can build all of corefx with Clang 10.
omajid added a commit to omajid/dotnet-corefx that referenced this pull request Apr 3, 2020
Clang 10 adds/enables new warnings, some of which is affecting
the corefx code.

Clang 10 has added -Walloca to warn about uses of alloca. This commit
replaces the only non-compliant use of that with a single fixed
stack-allocated buffer.

Clang 10 has also added -Wimplicit-int-float-conversion. This commit
uses explicit casts to double to avoid the warnings.

This is a backport of dotnet/runtime#33734 to corefx.

After this commit, I can build all of corefx with Clang 10.
omajid added a commit to omajid/dotnet-corefx that referenced this pull request Apr 14, 2020
Clang 10 adds/enables new warnings, some of which is affecting the
corefx code. Disable them.

This is a "backport" of dotnet/runtime#33734 to corefx.

After this commit, I can build all of corefx with Clang 10.
omajid added a commit to omajid/dotnet-corefx that referenced this pull request Apr 14, 2020
Clang 10 adds/enables new warnings, some of which is affecting the
corefx code. Disable them.

This is a "backport" of dotnet/runtime#33734 to corefx.

After this commit, I can build all of corefx with Clang 10.
omajid added a commit to omajid/dotnet-corefx that referenced this pull request Apr 14, 2020
Clang 10 adds/enables new warnings, some of which is affecting the
corefx code. Disable them.

This is a "backport" of dotnet/runtime#33734 to corefx.

After this commit, I can build all of corefx with Clang 10.
omajid added a commit to omajid/dotnet-corefx that referenced this pull request Apr 15, 2020
Clang 10 adds/enables new warnings, some of which is affecting the
corefx code. Disable them.

This is a "backport" of dotnet/runtime#33734 to corefx.

After this commit, I can build all of corefx with Clang 10.
omajid added a commit to omajid/dotnet-corefx that referenced this pull request Apr 15, 2020
Clang 10 adds/enables new warnings, some of which is affecting the
corefx code. Disable them.

This is a "backport" of dotnet/runtime#33734 to corefx.

After this commit, I can build all of corefx with Clang 10.
Anipik pushed a commit to dotnet/corefx that referenced this pull request May 13, 2020
Clang 10 adds/enables new warnings, some of which is affecting the
corefx code. Disable them.

This is a "backport" of dotnet/runtime#33734 to corefx.

After this commit, I can build all of corefx with Clang 10.
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
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.

failing to build runtime on clang 10
5 participants