win: Work towards getting 'minidump' to compile

win: Work towards getting 'minidump' to compile

- dbghelp.h requires windows.h to be included before it (ick!).
  Add a stub one for non_win to make this work.
- convert __attribute__ -> macro that can work work with MSVC;
- a handful of narrowing casts.

R=mark@chromium.org
BUG=crashpad:1
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/429a3368d4762a4bf7925400d2db584983819437

[scottmg, 2015-02-02 21:54:54]

It does not compile after this, but is much easier to parse the errors. Most of
the remaining errors are of the form:

d:\src\crashpad\crashpad\minidump\minidump_extensions.h(342) : warning C4200:
nonstandard extension used : zero-sized array in struct/union
        Cannot generate copy-ctor or copy-assignment operator when UDT contains
a zero-sized array
d:\src\crashpad\crashpad\minidump\minidump_rva_list_writer.h(70) : error C2229:
class 'crashpad::internal::MinidumpRVAListWriter' has an illegal zero-sized
array

Note that the second one is a hard error that can't be ignored. It seems like it
may require more invasive changes to resolve. Annoyingly enough, this applies
even to structures declared in Windows' dbghelp.h, for example

typedef struct _MINIDUMP_MODULE_LIST {
    ULONG32 NumberOfModules;
    MINIDUMP_MODULE Modules [ 0 ];
} MINIDUMP_MODULE_LIST, *PMINIDUMP_MODULE_LIST;

I guess they're only usable in plain-C contexts, not in C++ classes.

[Mark Mentovai, 2015-02-02 22:10:28]

Whoa, that <windows.h> thing is nuts. No IWYU there…

dbghelp.h disables C4200 for the structures it defines that make use of
zero-length arrays. I would have thought that C2229 would be addressed by
DISALLOW_COPY_AND_ASSIGN().

https://codereview.chromium.org/883773005/diff/20001/compat/win/sys/types.h
File compat/win/sys/types.h (left):

https://codereview.chromium.org/883773005/diff/20001/compat/win/sys/types.h#o...
compat/win/sys/types.h:24: typedef _W64 int ssize_t;
I thought _W64 sounded correct for this when you added it. It matched the
declaration of size_t from the SDK. No good?

https://codereview.chromium.org/883773005/diff/20001/minidump/minidump_context.h
File minidump/minidump_context.h (right):

https://codereview.chromium.org/883773005/diff/20001/minidump/minidump_contex...
minidump/minidump_context.h:257: PACK_AND_ALIGN_16(struct MinidumpContextAMD64 {
This may make it difficult for Doxygen to see the struct, not positive. We may
need

#ifdef DOXYGEN
#define BLAH(x) x
#endif

and that also might not work at all.

https://codereview.chromium.org/883773005/diff/20001/minidump/minidump_contex...
minidump/minidump_context.h:340: };)
The ;s should probably be outside the )s.

https://codereview.chromium.org/883773005/diff/20001/snapshot/process_snapshot.h
File snapshot/process_snapshot.h (right):

https://codereview.chromium.org/883773005/diff/20001/snapshot/process_snapsho...
snapshot/process_snapshot.h:24: #include <winsock2.h>
What for, timeval?

(Just for clarification. I don’t mean to add a comment for this, because the “//
for struct foo” definitions people sometimes add get stale very quickly.)

https://codereview.chromium.org/883773005/diff/20001/util/misc/struct_pack.h
File util/misc/struct_pack.h (right):

https://codereview.chromium.org/883773005/diff/20001/util/misc/struct_pack.h#...
util/misc/struct_pack.h:24: #define PACK_AND_ALIGN_N(decl, N) decl
__attribute__((packed, aligned(N)))
If you wind up with an #if defined(DOXYGEN) branch, you should put the
documentation for this new macro in that branch. If adding Doxygen documentation
for a macro, the file will also need an empty \file directive.
client/simulate_crash_mac.h is an example.

[scottmg, 2015-02-02 22:18:38]

On 2015/02/02 22:10:28, Mark Mentovai wrote:
> Whoa, that <windows.h> thing is nuts. No IWYU there…

After doing this, now I'm wondering if globally changing the usage to

#include "minidump/crashpad_dbghelp.h"

where that contains something like

#if defined(OS_WIN)
#include <windows.h>
#include <dbghelp.h>
#else
#include <dbghelp.h>
#endif

would be less ugly. Any preference?

> 
> dbghelp.h disables C4200 for the structures it defines that make use of
> zero-length arrays. I would have thought that C2229 would be addressed by
> DISALLOW_COPY_AND_ASSIGN().
> 
> https://codereview.chromium.org/883773005/diff/20001/compat/win/sys/types.h
> File compat/win/sys/types.h (left):
> 
>
https://codereview.chromium.org/883773005/diff/20001/compat/win/sys/types.h#o...
> compat/win/sys/types.h:24: typedef _W64 int ssize_t;
> I thought _W64 sounded correct for this when you added it. It matched the
> declaration of size_t from the SDK. No good?
> 
>
https://codereview.chromium.org/883773005/diff/20001/minidump/minidump_context.h
> File minidump/minidump_context.h (right):
> 
>
https://codereview.chromium.org/883773005/diff/20001/minidump/minidump_contex...
> minidump/minidump_context.h:257: PACK_AND_ALIGN_16(struct MinidumpContextAMD64
{
> This may make it difficult for Doxygen to see the struct, not positive. We may
> need
> 
> #ifdef DOXYGEN
> #define BLAH(x) x
> #endif
> 
> and that also might not work at all.
> 
>
https://codereview.chromium.org/883773005/diff/20001/minidump/minidump_contex...
> minidump/minidump_context.h:340: };)
> The ;s should probably be outside the )s.
> 
>
https://codereview.chromium.org/883773005/diff/20001/snapshot/process_snapshot.h
> File snapshot/process_snapshot.h (right):
> 
>
https://codereview.chromium.org/883773005/diff/20001/snapshot/process_snapsho...
> snapshot/process_snapshot.h:24: #include <winsock2.h>
> What for, timeval?
> 
> (Just for clarification. I don’t mean to add a comment for this, because the
“//
> for struct foo” definitions people sometimes add get stale very quickly.)
> 
> https://codereview.chromium.org/883773005/diff/20001/util/misc/struct_pack.h
> File util/misc/struct_pack.h (right):
> 
>
https://codereview.chromium.org/883773005/diff/20001/util/misc/struct_pack.h#...
> util/misc/struct_pack.h:24: #define PACK_AND_ALIGN_N(decl, N) decl
> __attribute__((packed, aligned(N)))
> If you wind up with an #if defined(DOXYGEN) branch, you should put the
> documentation for this new macro in that branch. If adding Doxygen
documentation
> for a macro, the file will also need an empty \file directive.
> client/simulate_crash_mac.h is an example.

[Mark Mentovai, 2015-02-02 22:24:43]

That would definitely consolidate the ugliness. I had hoped to just be able to
use native-like includes everywhere, but the SDK is kind of making that
difficult. I’d say that this sounds OK, but I’d share the <dbghelp.h> #include
outside of the #if.

[scottmg, 2015-02-02 22:31:45]

https://codereview.chromium.org/883773005/diff/20001/compat/win/sys/types.h
File compat/win/sys/types.h (left):

https://codereview.chromium.org/883773005/diff/20001/compat/win/sys/types.h#o...
compat/win/sys/types.h:24: typedef _W64 int ssize_t;
On 2015/02/02 22:10:28, Mark Mentovai wrote:
> I thought _W64 sounded correct for this when you added it. It matched the
> declaration of size_t from the SDK. No good?

Yeah, it does, I was just trying to avoid the need to include another header
here (windows.h) which could be confusing from sys/types.h.

Instead, I should have switched to __w64, which I've done now. Ref:
https://msdn.microsoft.com/en-us/library/s04b5w00.aspx

https://codereview.chromium.org/883773005/diff/20001/minidump/minidump_context.h
File minidump/minidump_context.h (right):

https://codereview.chromium.org/883773005/diff/20001/minidump/minidump_contex...
minidump/minidump_context.h:257: PACK_AND_ALIGN_16(struct MinidumpContextAMD64 {
On 2015/02/02 22:10:28, Mark Mentovai wrote:
> This may make it difficult for Doxygen to see the struct, not positive. We may
> need
> 
> #ifdef DOXYGEN
> #define BLAH(x) x
> #endif
> 
> and that also might not work at all.

Yeah, my ctags doesn't like it much either.

Now that I look at the instances given that there's only 2 headers doing this, a
better approach might be not to include the packing in the Windows macro, as
that's what requires the ugly syntax.

The alignment annotations can both occur in the same location, so given it could
instead look like:

#if defined(COMPILER_MSVC)
#pragma pack(push, 1)
#endif

struct ALIGNAS(4) POSIX_PACKED SomeStructure1 {
...
};

struct ALIGNAS(4) POSIX_PACKED SomeStructure2 {
...
};

#if defined(COMPILER_MSVC)
#pragma pack(pop)
#endif


Or ALIGNAS could include __attribute__((packed)) given a suitable name that
implies packed is only applied on non-MSVC. ALIGN_AS_4_AND_POSIX_PACKED ? :/

https://codereview.chromium.org/883773005/diff/20001/minidump/minidump_contex...
minidump/minidump_context.h:340: };)
On 2015/02/02 22:10:28, Mark Mentovai wrote:
> The ;s should probably be outside the )s.

Done.

https://codereview.chromium.org/883773005/diff/20001/snapshot/process_snapshot.h
File snapshot/process_snapshot.h (right):

https://codereview.chromium.org/883773005/diff/20001/snapshot/process_snapsho...
snapshot/process_snapshot.h:24: #include <winsock2.h>
On 2015/02/02 22:10:28, Mark Mentovai wrote:
> What for, timeval?

Right. It would probably be reasonable to add a compat/win/sys/time.h containing

struct timeval {
        long    tv_sec;         /* seconds */
        long    tv_usec;        /* and microseconds */
};


to avoid pulling in so much goop for so little.

> 
> (Just for clarification. I don’t mean to add a comment for this, because the
“//
> for struct foo” definitions people sometimes add get stale very quickly.)

[scottmg, 2015-02-02 22:47:27]

Patchset #4 (id:60001) has been deleted

[Mark Mentovai, 2015-02-02 22:57:21]

https://codereview.chromium.org/883773005/diff/20001/compat/win/sys/types.h
File compat/win/sys/types.h (left):

https://codereview.chromium.org/883773005/diff/20001/compat/win/sys/types.h#o...
compat/win/sys/types.h:24: typedef _W64 int ssize_t;
scottmg wrote:
> On 2015/02/02 22:10:28, Mark Mentovai wrote:
> > I thought _W64 sounded correct for this when you added it. It matched the
> > declaration of size_t from the SDK. No good?
> 
> Yeah, it does, I was just trying to avoid the need to include another header
> here (windows.h) which could be confusing from sys/types.h.
> 
> Instead, I should have switched to __w64, which I've done now. Ref:
> https://msdn.microsoft.com/en-us/library/s04b5w00.aspx

How does this work when _W64 didn’t? It looks like _W64 can only be __w64 or
blank.

https://codereview.chromium.org/883773005/diff/20001/minidump/minidump_context.h
File minidump/minidump_context.h (right):

https://codereview.chromium.org/883773005/diff/20001/minidump/minidump_contex...
minidump/minidump_context.h:257: PACK_AND_ALIGN_16(struct MinidumpContextAMD64 {
On 2015/02/02 22:31:45, scottmg wrote:
> On 2015/02/02 22:10:28, Mark Mentovai wrote:
> > This may make it difficult for Doxygen to see the struct, not positive. We
may
> > need
> > 
> > #ifdef DOXYGEN
> > #define BLAH(x) x
> > #endif
> > 
> > and that also might not work at all.
> 
> Yeah, my ctags doesn't like it much either.
> 
> Now that I look at the instances given that there's only 2 headers doing this,
a
> better approach might be not to include the packing in the Windows macro, as
> that's what requires the ugly syntax.
> 
> The alignment annotations can both occur in the same location, so given it
could
> instead look like:
> 
> #if defined(COMPILER_MSVC)
> #pragma pack(push, 1)
> #endif
> 
> struct ALIGNAS(4) POSIX_PACKED SomeStructure1 {
> ...
> };
> 
> struct ALIGNAS(4) POSIX_PACKED SomeStructure2 {
> ...
> };
> 
> #if defined(COMPILER_MSVC)
> #pragma pack(pop)
> #endif
> 
> 
> Or ALIGNAS could include __attribute__((packed)) given a suitable name that
> implies packed is only applied on non-MSVC. ALIGN_AS_4_AND_POSIX_PACKED ? :/

Ugly as it is, I think I prefer what you’ve got already, because it consolidates
the packiness and aligniness consistently for MSVC and non-MSVC. It seems kind
of bad to have to remember that you need both POSIX_PACKED and the MSVC #pragma
for consistency. Too bad MSVC doesn’t have a __declspec attribute for packiness.

Another alternative is to separate the top and bottom parts, so that you’d call
the PACK_AND_ALIGN(16) macro before a struct, and PACK_AND_ALIGN_END() after.
But this would require gcc/clang to permit the pack attribute to show up before
the struct keyword (maybe they do). It’d still be bad if someone wrote
PACK_AND_ALIGN(16) without PACK_AND_ALIGN_END(), which would mean different
things to gcc/clang and MSVC. I can’t say I like this alternative better, just
that it’s something else that occurred to me.

gcc and clang do recognize #pragma pack(push, x) and #pragma pack(pop). Maybe we
should get rid of the attributes altogether and use that directly. It’s an odd
construct, but we’re forced to support it at least for MSVC, so we might as well
get consistent behavior on all compilers.

https://codereview.chromium.org/883773005/diff/20001/snapshot/process_snapshot.h
File snapshot/process_snapshot.h (right):

https://codereview.chromium.org/883773005/diff/20001/snapshot/process_snapsho...
snapshot/process_snapshot.h:24: #include <winsock2.h>
scottmg wrote:
> On 2015/02/02 22:10:28, Mark Mentovai wrote:
> > What for, timeval?
> 
> Right. It would probably be reasonable to add a compat/win/sys/time.h
containing
> 
> struct timeval {
>         long    tv_sec;         /* seconds */
>         long    tv_usec;        /* and microseconds */
> };
> 
> 
> to avoid pulling in so much goop for so little.

Let’s do that, then.

[scottmg, 2015-02-02 23:16:37]

I made the macro change concurrent with your mail, so maybe take a look and see
what you think anyway. packed is only required in one file
(minidump/minidump_extensions.h) so it seems not too ugly this way. But I'm
happy to change back or to any other way.

https://codereview.chromium.org/883773005/diff/20001/compat/win/sys/types.h
File compat/win/sys/types.h (left):

https://codereview.chromium.org/883773005/diff/20001/compat/win/sys/types.h#o...
compat/win/sys/types.h:24: typedef _W64 int ssize_t;
On 2015/02/02 22:57:21, Mark Mentovai wrote:
> scottmg wrote:
> > On 2015/02/02 22:10:28, Mark Mentovai wrote:
> > > I thought _W64 sounded correct for this when you added it. It matched the
> > > declaration of size_t from the SDK. No good?
> > 
> > Yeah, it does, I was just trying to avoid the need to include another header
> > here (windows.h) which could be confusing from sys/types.h.
> > 
> > Instead, I should have switched to __w64, which I've done now. Ref:
> > https://msdn.microsoft.com/en-us/library/s04b5w00.aspx
> 
> How does this work when _W64 didn’t? It looks like _W64 can only be __w64 or
> blank.

That's right. __w64 is compiler defined, not by any header. _W64 is the Windows
SDK wrapper around that to avoid having to #if every time, but since we're
already doing that to change the size of the type, _W64 will always be __w64
here.

https://codereview.chromium.org/883773005/diff/100001/minidump/minidump_exten...
File minidump/minidump_extensions.h (right):

https://codereview.chromium.org/883773005/diff/100001/minidump/minidump_exten...
minidump/minidump_extensions.h:21: #include "minidump/crashpad_winnt.h"
Sadly, winnt.h has the same affliction as dbghelp.h. We could just hack #include
order in this one file instead in this case, as there's only one place where
winnt.h is included.

[Mark Mentovai, 2015-02-02 23:32:09]

https://codereview.chromium.org/883773005/diff/20001/compat/win/sys/types.h
File compat/win/sys/types.h (left):

https://codereview.chromium.org/883773005/diff/20001/compat/win/sys/types.h#o...
compat/win/sys/types.h:24: typedef _W64 int ssize_t;
On 2015/02/02 23:16:37, scottmg wrote:
> On 2015/02/02 22:57:21, Mark Mentovai wrote:
> > scottmg wrote:
> > > On 2015/02/02 22:10:28, Mark Mentovai wrote:
> > > > I thought _W64 sounded correct for this when you added it. It matched
the
> > > > declaration of size_t from the SDK. No good?
> > > 
> > > Yeah, it does, I was just trying to avoid the need to include another
header
> > > here (windows.h) which could be confusing from sys/types.h.
> > > 
> > > Instead, I should have switched to __w64, which I've done now. Ref:
> > > https://msdn.microsoft.com/en-us/library/s04b5w00.aspx
> > 
> > How does this work when _W64 didn’t? It looks like _W64 can only be __w64 or
> > blank.
> 
> That's right. __w64 is compiler defined, not by any header. _W64 is the
Windows
> SDK wrapper around that to avoid having to #if every time, but since we're
> already doing that to change the size of the type, _W64 will always be __w64
> here.

Right, my point is:

1. _W64 was always __w64 for us.

2. Something was broken and you removed _W64 to unbreak it.

3. Later, you switched this to __w64, and it remained unbroken.

But given (1), how is (3) any different than what we had before?

https://codereview.chromium.org/883773005/diff/160001/minidump/crashpad_dbghe...
File minidump/crashpad_dbghelp.h (right):

https://codereview.chromium.org/883773005/diff/160001/minidump/crashpad_dbghe...
minidump/crashpad_dbghelp.h:20: // dbghelp.h requires windows.h to be included
before it, so instead of
Other option: deal with this in compat/win and use the .. trick to simulate
#include_next. Not sure if that’s grosser.

https://codereview.chromium.org/883773005/diff/160001/minidump/minidump_conte...
File minidump/minidump_context.h (right):

https://codereview.chromium.org/883773005/diff/160001/minidump/minidump_conte...
minidump/minidump_context.h:257: struct ALIGNAS(16) MinidumpContextAMD64 {
Does ALIGNAS() set the packing too? I thought that you had to do both for MSVC.

[scottmg, 2015-02-03 18:12:56]

https://codereview.chromium.org/883773005/diff/20001/compat/win/sys/types.h
File compat/win/sys/types.h (left):

https://codereview.chromium.org/883773005/diff/20001/compat/win/sys/types.h#o...
compat/win/sys/types.h:24: typedef _W64 int ssize_t;
On 2015/02/02 23:32:08, Mark Mentovai wrote:
> On 2015/02/02 23:16:37, scottmg wrote:
> > On 2015/02/02 22:57:21, Mark Mentovai wrote:
> > > scottmg wrote:
> > > > On 2015/02/02 22:10:28, Mark Mentovai wrote:
> > > > > I thought _W64 sounded correct for this when you added it. It matched
> the
> > > > > declaration of size_t from the SDK. No good?
> > > > 
> > > > Yeah, it does, I was just trying to avoid the need to include another
> header
> > > > here (windows.h) which could be confusing from sys/types.h.
> > > > 
> > > > Instead, I should have switched to __w64, which I've done now. Ref:
> > > > https://msdn.microsoft.com/en-us/library/s04b5w00.aspx
> > > 
> > > How does this work when _W64 didn’t? It looks like _W64 can only be __w64
or
> > > blank.
> > 
> > That's right. __w64 is compiler defined, not by any header. _W64 is the
> Windows
> > SDK wrapper around that to avoid having to #if every time, but since we're
> > already doing that to change the size of the type, _W64 will always be __w64
> > here.
> 
> Right, my point is:
> 
> 1. _W64 was always __w64 for us.
> 
> 2. Something was broken and you removed _W64 to unbreak it.
> 
> 3. Later, you switched this to __w64, and it remained unbroken.
> 
> But given (1), how is (3) any different than what we had before?

Sorry, my explanations were too terse.

The original problem was that this header was incorrect -- it didn't #include
<windows.h>, so _W64 was only available sometimes by accident. But as it
happened, windows.h was always included before this file in all of our
translation units before this CL.

One fix would be to leave it as _W64 and include windows.h here. But that seems
heavy, since we know the value of _W64 will always be __w64 in this case.

https://codereview.chromium.org/883773005/diff/160001/minidump/crashpad_dbghe...
File minidump/crashpad_dbghelp.h (right):

https://codereview.chromium.org/883773005/diff/160001/minidump/crashpad_dbghe...
minidump/crashpad_dbghelp.h:20: // dbghelp.h requires windows.h to be included
before it, so instead of
On 2015/02/02 23:32:08, Mark Mentovai wrote:
> Other option: deal with this in compat/win and use the .. trick to simulate
> #include_next. Not sure if that’s grosser.

Maybe that's better. It seems a bit weird to have a shim on the platform where
the header originates. I guess that suggests another option -- an empty
windows.h in compat/non_win and then directly include <windows.h> before
dbghelp.h everywhere.

It seems a bit ugly too, but would match "standard" usage on Windows, and since
there's already a winnt.h doesn't seem so different from that.

https://codereview.chromium.org/883773005/diff/160001/minidump/minidump_conte...
File minidump/minidump_context.h (right):

https://codereview.chromium.org/883773005/diff/160001/minidump/minidump_conte...
minidump/minidump_context.h:257: struct ALIGNAS(16) MinidumpContextAMD64 {
On 2015/02/02 23:32:08, Mark Mentovai wrote:
> Does ALIGNAS() set the packing too? I thought that you had to do both for
MSVC.

No, but the original code didn't previously. Do you think that was mistakenly?

...

Looking through winnt.h in the Windows 8.1 SDK, it only packs the x86 CONTEXT
and leaves the x64 CONTEXT as naturally packed. So I think this matches the
layout there.

[Mark Mentovai, 2015-02-03 18:26:33]

https://codereview.chromium.org/883773005/diff/20001/compat/win/sys/types.h
File compat/win/sys/types.h (left):

https://codereview.chromium.org/883773005/diff/20001/compat/win/sys/types.h#o...
compat/win/sys/types.h:24: typedef _W64 int ssize_t;
On 2015/02/03 18:12:56, scottmg wrote:
> On 2015/02/02 23:32:08, Mark Mentovai wrote:
> > On 2015/02/02 23:16:37, scottmg wrote:
> > > On 2015/02/02 22:57:21, Mark Mentovai wrote:
> > > > scottmg wrote:
> > > > > On 2015/02/02 22:10:28, Mark Mentovai wrote:
> > > > > > I thought _W64 sounded correct for this when you added it. It
matched
> > the
> > > > > > declaration of size_t from the SDK. No good?
> > > > > 
> > > > > Yeah, it does, I was just trying to avoid the need to include another
> > header
> > > > > here (windows.h) which could be confusing from sys/types.h.
> > > > > 
> > > > > Instead, I should have switched to __w64, which I've done now. Ref:
> > > > > https://msdn.microsoft.com/en-us/library/s04b5w00.aspx
> > > > 
> > > > How does this work when _W64 didn’t? It looks like _W64 can only be
__w64
> or
> > > > blank.
> > > 
> > > That's right. __w64 is compiler defined, not by any header. _W64 is the
> > Windows
> > > SDK wrapper around that to avoid having to #if every time, but since we're
> > > already doing that to change the size of the type, _W64 will always be
__w64
> > > here.
> > 
> > Right, my point is:
> > 
> > 1. _W64 was always __w64 for us.
> > 
> > 2. Something was broken and you removed _W64 to unbreak it.
> > 
> > 3. Later, you switched this to __w64, and it remained unbroken.
> > 
> > But given (1), how is (3) any different than what we had before?
> 
> Sorry, my explanations were too terse.
> 
> The original problem was that this header was incorrect -- it didn't #include
> <windows.h>, so _W64 was only available sometimes by accident. But as it
> happened, windows.h was always included before this file in all of our
> translation units before this CL.
> 
> One fix would be to leave it as _W64 and include windows.h here. But that
seems
> heavy, since we know the value of _W64 will always be __w64 in this case.

Got it! Thanks for the explanation.

https://codereview.chromium.org/883773005/diff/160001/minidump/crashpad_dbghe...
File minidump/crashpad_dbghelp.h (right):

https://codereview.chromium.org/883773005/diff/160001/minidump/crashpad_dbghe...
minidump/crashpad_dbghelp.h:20: // dbghelp.h requires windows.h to be included
before it, so instead of
On 2015/02/03 18:12:56, scottmg wrote:
> On 2015/02/02 23:32:08, Mark Mentovai wrote:
> > Other option: deal with this in compat/win and use the .. trick to simulate
> > #include_next. Not sure if that’s grosser.
> 
> Maybe that's better. It seems a bit weird to have a shim on the platform where
> the header originates. I guess that suggests another option -- an empty
> windows.h in compat/non_win and then directly include <windows.h> before
> dbghelp.h everywhere.
> 
> It seems a bit ugly too, but would match "standard" usage on Windows, and
since
> there's already a winnt.h doesn't seem so different from that.

I’ll leave it up to your discretion. All of the options have benefits and
drawbacks.

https://codereview.chromium.org/883773005/diff/160001/minidump/minidump_conte...
File minidump/minidump_context.h (right):

https://codereview.chromium.org/883773005/diff/160001/minidump/minidump_conte...
minidump/minidump_context.h:257: struct ALIGNAS(16) MinidumpContextAMD64 {
On 2015/02/03 18:12:56, scottmg wrote:
> On 2015/02/02 23:32:08, Mark Mentovai wrote:
> > Does ALIGNAS() set the packing too? I thought that you had to do both for
> MSVC.
> 
> No, but the original code didn't previously. Do you think that was mistakenly?

Ah, you’re right.

> Looking through winnt.h in the Windows 8.1 SDK, it only packs the x86 CONTEXT
> and leaves the x64 CONTEXT as naturally packed. So I think this matches the
> layout there.

https://codereview.chromium.org/883773005/diff/160001/minidump/minidump_exten...
File minidump/minidump_extensions.h (right):

https://codereview.chromium.org/883773005/diff/160001/minidump/minidump_exten...
minidump/minidump_extensions.h:28: #pragma pack(push, 1)
This is going to suck if we want a non-packed struct in here, but that should
hopefully never happen.

https://codereview.chromium.org/883773005/diff/160001/minidump/minidump_exten...
minidump/minidump_extensions.h:315: );
Make this a } again.

[scottmg, 2015-02-03 19:22:44]

https://codereview.chromium.org/883773005/diff/160001/minidump/crashpad_dbghe...
File minidump/crashpad_dbghelp.h (right):

https://codereview.chromium.org/883773005/diff/160001/minidump/crashpad_dbghe...
minidump/crashpad_dbghelp.h:20: // dbghelp.h requires windows.h to be included
before it, so instead of
On 2015/02/03 18:26:32, Mark Mentovai wrote:
> On 2015/02/03 18:12:56, scottmg wrote:
> > On 2015/02/02 23:32:08, Mark Mentovai wrote:
> > > Other option: deal with this in compat/win and use the .. trick to
simulate
> > > #include_next. Not sure if that’s grosser.
> > 
> > Maybe that's better. It seems a bit weird to have a shim on the platform
where
> > the header originates. I guess that suggests another option -- an empty
> > windows.h in compat/non_win and then directly include <windows.h> before
> > dbghelp.h everywhere.
> > 
> > It seems a bit ugly too, but would match "standard" usage on Windows, and
> since
> > there's already a winnt.h doesn't seem so different from that.
> 
> I’ll leave it up to your discretion. All of the options have benefits and
> drawbacks.

I went with adding an empty non_win/windows.h as it seems the least "tricky".

https://codereview.chromium.org/883773005/diff/160001/minidump/minidump_exten...
File minidump/minidump_extensions.h (right):

https://codereview.chromium.org/883773005/diff/160001/minidump/minidump_exten...
minidump/minidump_extensions.h:28: #pragma pack(push, 1)
On 2015/02/03 18:26:32, Mark Mentovai wrote:
> This is going to suck if we want a non-packed struct in here, but that should
> hopefully never happen.

Agreed.

[Mark Mentovai, 2015-02-03 19:31:22]

LGTM

[scottmg, 2015-02-03 19:37:58]

New patchsets have been uploaded after l-g-t-m from mark@chromium.org

[scottmg, 2015-02-03 19:38:08]

Patchset #10 (id:200001) has been deleted

[scottmg, 2015-02-05 01:30:08]

Committed patchset #9 (id:180001) manually as
429a3368d4762a4bf7925400d2db584983819437 (presubmit successful).