win: Work towards getting 'minidump' to compile

minidump/minidump_context.h

Index: minidump/minidump_context.h
diff --git a/minidump/minidump_context.h b/minidump/minidump_context.h
index 9da4c44b534644653548d7f1686de1223d9cde7f..46f53cb2e45263087912aa88bac319f00e51dfea 100644
--- a/minidump/minidump_context.h
+++ b/minidump/minidump_context.h
@@ -18,6 +18,7 @@
 #include <stdint.h>
 
 #include "snapshot/cpu_context.h"
+#include "util/misc/struct_pack.h"
 #include "util/numeric/int128.h"
 
 namespace crashpad {
@@ -187,7 +188,7 @@ struct MinidumpContextX86 {
   CPUContextX86::Fxsave fxsave;
 };
 
-//! \brief x86_64-specifc flags for MinidumpContextAMD64::context_flags.
+//! \brief x86_64-specific flags for MinidumpContextAMD64::context_flags.
 enum MinidumpContextAMD64Flags : uint32_t {
   //! \brief Identifies the context structure as x86_64. This is the same as
   //!     `CONTEXT_AMD64` on Windows for this architecture.
@@ -253,7 +254,7 @@ enum MinidumpContextAMD64Flags : uint32_t {
 //!     normally alias `dr6` and `dr7`, respectively. See Intel Software
 //!     Developer’s Manual, Volume 3B: System Programming, Part 2 (253669-052),
 //!     17.2.2 “Debug Registers DR4 and DR5”.
-struct __attribute__((aligned(16))) MinidumpContextAMD64 {
+PACK_AND_ALIGN_16(struct MinidumpContextAMD64 {

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

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.

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

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 ? :/

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

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.

   //! \brief Register parameter home address.
   //!
   //! On Windows, this field may contain the “home” address (on-stack, in the
@@ -336,7 +337,7 @@ struct __attribute__((aligned(16))) MinidumpContextAMD64 {
   uint64_t last_exception_to_rip;
   uint64_t last_exception_from_rip;
   //! \}
-};
+};)

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

The ;s should probably be outside the )s.

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

Done.

 
 }  // namespace crashpad