Fix non standard C++ code constructs on Windows by sesmith177 · Pull Request #4645 · envoyproxy/envoy

sesmith177 · 2018-10-08T21:24:38Z

Description:

As requested in #129, this is the first in a series of changes that will lead to Envoy building and running on Windows. To start, this PR fixes some non standard C++ code constructs that are not supported by MSVC:

Use STACK_ALLOC_ARRAY macro to allocate a variable-length array on the stack
Don't use designated initializers with structs
Remove usage of ?: operator
Use PACKED_STRUCT macro to typedef a packed struct

These changes are necessary, but not sufficient, to get Envoy to compile on Windows

Risk Level:
Low

Testing:
bazel build //source/exe:envoy-static && bazel test //test/...

Docs Changes:
N/A

Release Notes:
N/A

1. Use STACK_ALLOC_ARRAY macro to allocate a variable-length array on the stack 2. Don't use designated initializers with structs 3. Remove usage of ?: operator 4. Use PACKED_STRUCT macro to typedef a packed struct Signed-off-by: Sam Smith <sesmith177@gmail.com>

Signed-off-by: Andrew Keesler <akeesler@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

alyssawilk

Goodness, thanks for chasing all of these down!

Any chance you can find a regex which will catch most of these when folks do their fix_format? I'm concerned that we may otherwise end up with a game of whack-a-mole until we have a windows CI up and running, and it'd be nice if folks could catch these issues before hitting CI in any case.

dnoe · 2018-10-09T13:51:17Z

source/common/compressor/zlib_compressor_impl.cc


-  for (const Buffer::RawSlice& input_slice : slices) {
+  for (uint64_t i = 0; i < num_slices; i++) {
+    Buffer::RawSlice& input_slice = slices[i];


const Buffer::RawSlice&?

lizan · 2018-10-09T19:23:23Z

include/envoy/common/platform.h

+#pragma once
+
+// NOLINT(namespace-envoy)
+#if !defined(WIN32)


I think those are compiler dependent so it should be !defined(_MSC_VER)? If someone uses LLVM on Windows then those should work.

- should fix build on OSX - add back missing const - STACK_ALLOC_ARRAY and PACKED_STRUCT macros based on compiler, not OS Signed-off-by: Arjun Sreedharan <asreedharan@pivotal.io>

- add checks for ?:, __attribute__((packed)) and designated initializers - fix additional usages found by these checks Signed-off-by: Sam Smith <sesmith177@gmail.com>

sesmith177 · 2018-10-10T17:56:57Z

@alyssawilk we've added some checks to check_format.py, which actually turned up a couple usages that we had not found yet (due to not compiling hot restart code on Windows).

We didn't add a check for the variable length arrays because given:

type name[num];

it is not possible to tell by regex if num is a variable or a constant

alyssawilk · 2018-10-10T18:10:49Z

Can't we do best effort based on the Envoy style rules? If it's based on a constant it should be buf[FOO] or buf[FooLen] vs buf[foo_len] for variable length. I'm not entirely sure how consistent we are but worth giving it a try?

Signed-off-by: Arjun Sreedharan <asreedharan@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

sesmith177 · 2018-10-10T18:58:35Z

It looks like there's still the case where the variable is declared const (e.g.

envoy/source/common/runtime/runtime_impl.cc

Lines 49 to 50 in e91ae86

    
           const size_t prefetch = 256; 
        
           static thread_local uint64_t buffered[prefetch];

) or constexpr (e.g.

envoy/source/common/common/perf_annotation.cc

Lines 84 to 85 in e91ae86

    
           constexpr int num_columns = ARRAY_SIZE(headers); 
        
           size_t widths[num_columns];

)

A simple regex would also flag return buf[foo];, though that case could be handled

alyssawilk

Argh, makes sense. Thanks for giving it a shot in any case, and I'm glad the other ones proved a bit helpful!

@mattklein123 @ggreenway @zuercher anyone up for a second pass?

alyssawilk · 2018-10-10T19:51:24Z

Also the coverage build has been fixed, but you're going to need to merge master in order to pick the fix up.

Signed-off-by: Arjun Sreedharan <asreedharan@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

lizan · 2018-10-10T22:26:51Z

@sesmith177 @alyssawilk the check could be done via compiler flags to make it fail (at least for clang), use -Wvla-extensions and -Wgnu-conditional-omitted-operand?

sesmith177 · 2018-10-11T14:02:17Z

@lizan we could add -Wgnu-conditional-omitted-operand to the clang compiler options. I don't think we could use -Wvla-extension with this PR as is, since we're still using variable-length arrays, just hiding them behind a macro

Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Arjun Sreedharan <asreedharan@pivotal.io>

mattklein123

Thank you, LGTM!

mattklein123 · 2018-10-11T18:20:00Z

@lizan can you merge if you are satisfied?

jmarantz · 2018-10-15T21:44:28Z

include/envoy/common/platform.h

+#include <malloc.h>
+
+#define STACK_ALLOC_ARRAY(var, type, num)                                                          \
+  type* var = static_cast<type*>(::_alloca(sizeof(type) * num))


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.

you could loop over the array and used placed new, FWIW. You'd also need to take care of destructors.

@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::RawSlice

struct iovec

uint64_t

unsigned char

and none of these types have constructors/destructors that do anything interesting. In the Buffer::RawSlice + struct iovec case, it looks like we always manually initialize the array (either through getRawSlices or looping over the array).

The best way to fix this probably depends on:

how important it is that these arrays are on the stack

does the solution need to be fully general or could we restrict it to the types above

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?

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?

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).

A third option might be to use something like:

static_assert(std::is_trivially_destructible<type>::value, #type " must be trivially destructible");

in the macro/wrapper class to ensure that type has 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.

@mattklein123 @jmarantz We have a branch that replaces variable length arrays with std::vector (and adds -Wvla to ensure variable length arrays are invalid) here: https://github.com/greenhouse-org/envoy/commit/47c48a48f4ea71c00eb714674094bbd6d3d2a4c1

If it's decided that these arrays must be allocated on the stack, we will work on a wrapper template object and PR that instead

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?

Sounds good, we'll get that PR'd

Andrew Keesler and others added 2 commits October 8, 2018 17:15

Fix format

7a380e9

Signed-off-by: Andrew Keesler <akeesler@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

alyssawilk suggested changes Oct 9, 2018

View reviewed changes

alyssawilk self-assigned this Oct 9, 2018

dnoe reviewed Oct 9, 2018

View reviewed changes

lizan reviewed Oct 9, 2018

View reviewed changes

sesmith177 and others added 2 commits October 10, 2018 10:32

Address feedback:

f3ff12a

- should fix build on OSX - add back missing const - STACK_ALLOC_ARRAY and PACKED_STRUCT macros based on compiler, not OS Signed-off-by: Arjun Sreedharan <asreedharan@pivotal.io>

Add check_format checks for non-standard code

62e37ee

- add checks for ?:, __attribute__((packed)) and designated initializers - fix additional usages found by these checks Signed-off-by: Sam Smith <sesmith177@gmail.com>

Loosen designated initializer regex pattern

10edd4f

Signed-off-by: Arjun Sreedharan <asreedharan@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

alyssawilk previously approved these changes Oct 10, 2018

View reviewed changes

Merge branch 'master' into msvc-changes

d9faa41

Signed-off-by: Arjun Sreedharan <asreedharan@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>

alyssawilk assigned lizan Oct 11, 2018

dnoe previously approved these changes Oct 11, 2018

View reviewed changes

Make use of ?: an error on clang

9b78956

Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Arjun Sreedharan <asreedharan@pivotal.io>

sesmith177 dismissed stale reviews from dnoe and alyssawilk via 9b78956 October 11, 2018 14:22

mattklein123 approved these changes Oct 11, 2018

View reviewed changes

lizan approved these changes Oct 11, 2018

View reviewed changes

lizan merged commit 8cd9c69 into envoyproxy:master Oct 11, 2018

jmarantz reviewed Oct 15, 2018

View reviewed changes

sesmith177 mentioned this pull request Oct 26, 2018

remove use of variable length arrays #4870

Merged

sesmith177 deleted the msvc-changes branch November 28, 2018 20:56

sesmith177 mentioned this pull request Dec 14, 2018

thread: add Windows implementation #5072

Merged

Conversation

sesmith177 commented Oct 8, 2018

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sesmith177 commented Oct 10, 2018

Uh oh!

alyssawilk commented Oct 10, 2018

Uh oh!

sesmith177 commented Oct 10, 2018

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Oct 10, 2018

Uh oh!

lizan commented Oct 10, 2018

Uh oh!

sesmith177 commented Oct 11, 2018

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Oct 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants