SimonKagstrom / kcov

Code coverage tool for compiled programs, Python and Bash which uses debugging information to collect and report data without special compilation options
http://simonkagstrom.github.io/kcov/
GNU General Public License v2.0
712 stars 109 forks source link

A couple of -Waddress-of-packed-member warnings #311

Closed dridi closed 5 years ago

dridi commented 5 years ago

I'm building kcov with GCC 9.2.1 and get the following warnings:

src/merge-file-parser.cc: In member function ‘const file_data* MergeParser::marshalFile(const string&)’:
src/merge-file-parser.cc:459:31: warning: taking address of packed member of ‘file_data’ may result in an unaligned pointer value [-Waddress-of-packed-member]
  459 |   struct line_entry *p = out->entries;
      |                          ~~~~~^~~~~~~
src/merge-file-parser.cc: In member function ‘bool MergeParser::unMarshalFile(file_data*)’:
src/merge-file-parser.cc:524:30: warning: taking address of packed member of ‘file_data’ may result in an unaligned pointer value [-Waddress-of-packed-member]
  524 |   struct line_entry *p = fd->entries;
      |                          ~~~~^~~~~~~

They appear because struct line_entry is packed and loses alignment guarantees for the flex-array entries. The warning goes away with a simple change:

diff --git i/src/merge-file-parser.cc w/src/merge-file-parser.cc
index 30664d9..f91d39c 100644
--- i/src/merge-file-parser.cc
+++ w/src/merge-file-parser.cc
@@ -43,7 +43,7 @@ struct file_data
        uint32_t file_name_offset;

        struct line_entry entries[];
-}__attribute__((packed));
+};

 // Unit test stuff
 namespace merge_parser

However I'm not sure how safe it is to make this change since the result is serialized. Could the structure be written as-is today and later be (mis)read by a patched kcov after an upgrade?

An alternative would be keep the packed structure and allocate a temporary line entry on the stack, and memcpy its contents for each entry: we get both a packed structure and alignment guarantees. However this doesn't work if entries are manipulated in-memory and not only to be serialized on disk (which I didn't verify).

This is generally not a problem on x86 hardware but could be on other architectures.

SimonKagstrom commented 5 years ago

Well, the packing is intentional, but can probably be removed. It might save some space, but that's quite minor.

As long as it's read the same way as it's written, it shouldn't really matter if it's unpacked (and I guess therefore might differ between compilers). kcov reads and writes this data all by itself, so I don't see any disadvantages with removing it.

Thanks!

SimonKagstrom commented 5 years ago

Care to submit a pull request?

dridi commented 5 years ago

Sure thing, I submitted #312.

SimonKagstrom commented 5 years ago

Great, thanks a lot!