-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix non standard C++ code constructs on Windows #4645
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
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
b679383
Fix non-C++ standard code constructs on Windows
7a380e9
Fix format
sesmith177 f3ff12a
Address feedback:
sesmith177 62e37ee
Add check_format checks for non-standard code
10edd4f
Loosen designated initializer regex pattern
sesmith177 d9faa41
Merge branch 'master' into msvc-changes
sesmith177 9b78956
Make use of ?: an error on clang
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| #pragma once | ||
|
|
||
| // NOLINT(namespace-envoy) | ||
| #if !defined(_MSC_VER) | ||
| #define STACK_ALLOC_ARRAY(var, type, num) type var[num] | ||
|
|
||
| #define PACKED_STRUCT(definition, ...) definition, ##__VA_ARGS__ __attribute__((packed)) | ||
|
|
||
| #else | ||
| #include <malloc.h> | ||
|
|
||
| #define STACK_ALLOC_ARRAY(var, type, num) \ | ||
| type* var = static_cast<type*>(::_alloca(sizeof(type) * num)) | ||
|
|
||
| #define PACKED_STRUCT(definition, ...) \ | ||
| __pragma(pack(push, 1)) definition, ##__VA_ARGS__; \ | ||
| __pragma(pack(pop)) | ||
|
|
||
| #endif | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I just noticed this...does this actually work? What if 'type' has a constructor; it doesn't look like it would be called and you'd get uninitialized memory.
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.
you could loop over the array and used placed new, FWIW. You'd also need to take care of destructors.
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.
@jmarantz that's a good point -- the constructors/destructors definitely won't get called.
It works for now since the macro is only used with:
Buffer::RawSlicestruct iovecuint64_tunsigned charand none of these types have constructors/destructors that do anything interesting. In the
Buffer::RawSlice+struct ioveccase, it looks like we always manually initialize the array (either throughgetRawSlicesor looping over the array).The best way to fix this probably depends on:
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.
Great catch @jmarantz. @sesmith177 we definitely need to fix this somehow as it will produce a bunch of annoying and hard to find bugs. One option that comes to mind is having some wrapper RAII template object that does placement new and then would automatically do placement delete on scope exit?
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.
As an aside, could we just say that users have to compile Envoy with clang on windows? I'm wondering if that would also be a way to "fix" these problems?
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.
Another alternative here is to just edit the code-sites to just not use stack allocation. Is there a perf win from stack in this case? My thinking in general has been that if you want to have a lot of threads, then you don't want to allocate huge amounts of stack space to them, so you might not want to allocate potentially large things on the stack.
If that sounds right, I'd seal the deal by throwing a compile switch in clang to make that invalid, if such a switch exists (IIRC g++ has such a switch).
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.
A third option might be to use something like:
in the macro/wrapper class to ensure that
typehas a trivial destructor and doesn't need to be deleted.We're happy to go in any of the above directions.
It would be a significant effort to require clang on Windows. The tricky part would be ensuring that all third-party dependencies can be build with clang on Windows as well (i.e. do not require MSVC). When initially starting on this project, we made an attempt to compile everything with gcc/mingw, unfortunately there were dependencies that would not build on Windows with that toolchain.
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.
@mattklein123 @jmarantz We have a branch that replaces variable length arrays with
std::vector(and adds-Wvlato ensure variable length arrays are invalid) here: https://github.com/greenhouse-org/envoy/commit/47c48a48f4ea71c00eb714674094bbd6d3d2a4c1If it's decided that these arrays must be allocated on the stack, we will work on a wrapper template object and PR that instead
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.
I'm not in favor of switching to std::vector as I don't think we should take a perf hit due to a platform issue. If it's not too bad, I think I would go with the wrapper template. I don't think it will be that hard so can we try it first?
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.
Sounds good, we'll get that PR'd