Fix src/libraries to build on clang 10#33734
Conversation
|
cc @jkotas |
There was a problem hiding this comment.
| int useStackBuffer = bufferSize <= 2048; | |
| int useStackBuffer = bufferSize <= sizeof(stackBuffer); |
... and move it after stackBuffer.
b8d12fd to
7223624
Compare
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Ah, that's pseudo-code. You can probably use a #define here to work around the warning instead of disabling the warning.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Replacing malloc with calloc is good, but I would undo the split into two methods.
There was a problem hiding this comment.
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".)
06075ca to
69ceb82
Compare
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.
69ceb82 to
00a63bb
Compare
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.
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.
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.
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.
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.
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.
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.
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.
Clang 10 adds/enables new warnings, some of which is affecting thesrc/libraries code.
Clang 10 has added
-Wallocato warn about uses ofalloca. 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.