15

So, I encountered a weird bug at work when I was trying to reduce the size of a struct by using bit-fields. I managed to isolate the problem and produce a minimal version that replicates the issue. This had the added benefit that MSVC now even warns about what it is going to do: Link to Compiler Explorer. However, Clang and GCC have no problems with this code.

#include <new>

enum foo : unsigned char { zero, one };

struct S
{
    int a{ 42 }; //Intentionally not trivial
    foo b : 1;
    foo c : 1;
};

auto test()
{
    constexpr auto size      = sizeof (S);
    constexpr auto alignment = alignof(S);

    struct {
        alignas(alignment) unsigned char bytes[size];
    } data;

    //Buffer overrun on this line
    ::new(static_cast<void*>(&data)) S{};

    //Just to avoid optimizing away the offending instructions
    return data;
}

The buffer that I am using should be suitable for storing the object, as it mimics std::aligned_storage, and I am invoking True Placement New to store my object in it. I believe this is how f.ex std::vector works. Despite this, I get this warning:

warning C4789: buffer 'data' of size 8 bytes will be overrun; 5 bytes will be written starting at offset 4

The weird thing is the problem goes away if the braces are replaced with parenthesis (should still be value initialized though, right?) or just removed entirely (default initialized).

The problem also goes away if S is trivial. Additionally, the problem only occurs with optimizations enabled (had a fun debugging experience because of that).

I do not believe I am invoking undefined behavior here, but the alternative is that there is a bug in VS 2017 and 2019. Currently I am working around the issue by just not using braced initialization and sticking with parenthesis where I suspect there could be issues, but this feels wrong.

Why is this happening, and what can I do to avoid worrying about time bombs in my code? Switching to another compiler is not an option.

Update: So, looking a bit more at the assembly I see that it's still generating suspicious code when using parenthesis to trigger value initialization. Only default initialization produces the expected assembly. Seems quite odd, and I'm definitely suspecting a compiler bug, but I would prefer some more input on this.

Bendik Hillestad
  • 191
  • 8
  • "The weird thing is the problem goes away if the braces are replaced with parenthesis (should still be value initialized though, right?) or just removed entirely (default initialized)." That's probably weird enough to signal a bug. – L. F. Sep 20 '19 at 08:55
  • @L.F. I am suspecting that, but I'm still not confident enough regarding initialization rules in modern c++, so I can't say for certain if there's a small subtlety between () and {} in this case. – Bendik Hillestad Sep 20 '19 at 09:38
  • Oh and I am also not entirely sure how to go about reporting this as a bug to the developers of Visual Studio. – Bendik Hillestad Sep 20 '19 at 09:45
  • I have reported the problem [here](https://developercommunity.visualstudio.com/content/problem/741580/incorrect-codegen-causes-buffer-overrun-with-brace.html). Hopefully I did it correctly. – Bendik Hillestad Sep 20 '19 at 11:52
  • You can link back to this Stack Overflow question from the bug report. – L. F. Sep 20 '19 at 12:47
  • @L.F. alright I did that now. – Bendik Hillestad Sep 20 '19 at 13:38
  • looks like the ‘alignas‘ keyword is being ignored leading to ‘data‘ being placed at offset 4 and making first 4 bytes wasted. Did you check the compiler switches that may affect the keyword? – Red.Wave Sep 20 '19 at 17:44
  • by the way, why do you not use ’std::aligned_storage‘? – Red.Wave Sep 20 '19 at 17:47
  • @Red.Wave the alignment is not being ignored, this can be observed by inspecting the `bytes` array. The value 42 (due to `int a{ 42 }`) is written at `bytes[0]`. The 5 bytes that get written starting at offset 4 are just zeroes. I am not using `std::aligned_storage` because it's likely getting deprecated, and it's so trivial to write yourself using only the core language. Additionally I want reliable access to the underlying bytes, and not all compilers call that member the same thing. – Bendik Hillestad Sep 21 '19 at 09:55
  • @BendikHillestad I had difficulty understanding your last sentence about name. By the way, if you need byte-level access, type-punning can still be implemented in a couple of ways. Moreover, to check the alignment, the address - not the value contained - should be observed. – Red.Wave Sep 21 '19 at 14:54
  • Set either of those flags in ctor to check what happened. – Red.Wave Sep 21 '19 at 15:10
  • `S` is an aggregate, `S{}` is an aggregate initialization so `b` and `c` members are default-initialized. `S()` performs a value initialization, so `b` and `c` are not initialized. – Oliv Sep 21 '19 at 15:51
  • @Red.Wave I am referring to the `unsigned char bytes[size];`, in GCC/Clang that's `_Bytes` and in MSVC it's `_Pad`. Point is that getting access to the bytes when using `std::aligned_storage` requires hacks, and just in general I get more problems than benefits from using that as opposed to writing it myself. Also, the address is correct. However, on x86 this doesn't really matter. You can change the code above to `alignas(1)` and it still does the same. No bytes are wasted, there's just a single zero being written past the end. If we change the size to `size + 1` there's no longer a problem. – Bendik Hillestad Sep 22 '19 at 13:48
  • @Oliv b and c should be either zero-initialized or default-initialization regardless. – Bendik Hillestad Sep 22 '19 at 14:16
  • @BendikHillestad I don't argue against the existence of a compiler bug; I am rather curious to know the probable reason. – Red.Wave Sep 22 '19 at 17:08
  • @Red.Wave I am aware. I probably should have pointed out that I already spent hours before posting this exhaustively ruling out certain things, like alignment issues and the compiler starting to write from the wrong address. What is clear is that it really _is_ writing 9 bytes, but reports the size as 8 bytes. My best guess at the moment is that the compiler gets confused by the bit-fields and incorrectly pads the struct, leading to what should be a "4 bytes will be written starting at offset 4" into a "5 bytes will be written starting at offset 4". – Bendik Hillestad Sep 23 '19 at 05:54
  • I have made a slightly clearer example [here](https://gcc.godbolt.org/z/nVDDtH). The assembly translates to this: Write one byte with the value 42, then 7 bytes of zeroes, and then 1 byte of whatever register 'al' happens to contain (which in my tests has always been another zero. I'm not that experienced with assembly, but maybe copying 'al' in this case is an optimization over literally writing zero?). – Bendik Hillestad Sep 23 '19 at 06:07
  • Note that the warning is now gone in that version (but the buffer overrun still occurs). Also I was blind: The first instruction `xor eax, eax` guarantees 'al' to be zero. Also of note: Replacing the `S{}` with `S()` confusingly zero-initializes the buffer first and then immediately writes the expected sequence of 42 followed by 7 bytes of zeroes. Doing just `S` gives us the expected single-instruction function. – Bendik Hillestad Sep 23 '19 at 06:24
  • Okay wow, so before I posted this whole question I tried first to see if I could trigger this issue without having the buffer to construct into, but I must have been fighting the optimizer the whole time. I was now able to trigger the issue with a [_very small_ example](https://gcc.godbolt.org/z/8KNhVi). I think this clears any fear that it might be related to weird buffers or addresses. – Bendik Hillestad Sep 23 '19 at 07:17
  • I have to make with this nasty cell phone for a while. Otherwise I was no less crazy to test this code in various ways. Wrong arguments to optimized away ‘memset’? Did you consider defining none-zero-initialized variables before and after the buffer - literaly enclosing it between two words - to inspect the real damage due to buffer overrun? – Red.Wave Sep 23 '19 at 18:35
  • 1
    Yes, when I initially discovered the bug I was writing into a big array of other data. In my particular case it wrote a single zero byte over some other data next to it in memory. That data happened to be important enough that my program crashed quite fast. Debugging was still a pain however, especially when it stopped overwriting memory in debug mode. – Bendik Hillestad Sep 23 '19 at 20:57

1 Answers1

3

This compiler bug is being fixed in VS 2019 16.5.

As a workaround for those who cannot upgrade, consider replacing braces with regular parenthesis, eg replace S{...}; with S(...);. If no arguments are being supplied to the constructor, consider simply removing the braces as the object is still default-constructed this way.

Bendik Hillestad
  • 191
  • 8