[haiku-commits] Re: haiku: hrev52133 - src/apps/mediaconverter

  • From: Adrien Destugues <pulkomandy@xxxxxxxxxxxxx>
  • To: haiku-commits@xxxxxxxxxxxxx
  • Date: Fri, 27 Jul 2018 08:12:54 +0200

On Thu, Jul 26, 2018 at 11:25:50PM +0200, Stephan Aßmus wrote:

Thanks for the info. Isn't a "private:" declaration just a compile time
thing? If so, that would leave "overload of assignment" as a potential
reason for why media_format may not be "trivially copyable". I don't really
see how, though, i.e. why that would affect object storage. But I probably
just don't know enough about C++ implementation details. Or is there more
hiding in that "etc"?

Again, thanks for all the info, I've learned a lot. I guess that's what I
get for being mostly self-taught. IIRC, I even learned about unions mostly
from seeing how media_format works. What I don't understand is how code in
one place can even know which union member has been initialized in another
place of the code. It would seem to make it necessary to always have fields
in unions wich carry this information (such as media_format::type). But I
don't mean for you to keep digging, I should do that myself.

In any case, in my previous mails, I think more than once, I was asking the
honest question if memset() may have unwanted side-effects, or be
ineffective at fully initializing the union. A simple "Yes, please google
it" would have been fine for me. And in fact I did google it, but only found
stuff that confirmed what I learned before. (In essence, that media_format
should be "trivially copyable".) Alter I saw that media_format also has the
meta_data stuff, which makes it not trivially copyable, but that was taken
into account where memset() was used, although not always in the correct
way.

There are two ways to look at this, in fact.

One is what the standard says. Since C++ is a "design by committee"
language, there are a lot of people involved in the discussion and they
tend to overthink things with a lot of "what if?" cases. They are also
trying to let compiler implementations decide how exactly they implement
things.

As a result, the definition of POD in the standard has become very
restrictive. For example, as soon as a field is private, the compiler
knows that nothing outside the class would try to play dirty things like
memsetting it or so. As a result, they could lift the requirement that
the public and private members of the class are stored side by side.

Writing fully standard C++ conforming code is thus a more difficult
task, because in a lot of places you have to think about things like
that.

However, we have a second level of standard: the Itanium ABI, which is
used by GCC. This is where we can look when we want to know how a class
is stored in memory, what is a vtable, etc. And this allows us to find a
reasonable behavior for things that the C++ standard says are
"implementation defined", and in some cases "undefined behavior".

Then, we also have to keep in mind that for the "undefined behavior"
parts, while the code we write (in this case, a memset in an object
which seems to have a plain and simple memory representation) may change
over time as the compiler performs more and more subtle optimizations.
If nothing in the standard says that the memset is allowed, gcc may at
some point try to outsmart us and simply remove it from the code, all
while still perfectly following the C++ standard.

This already happened to us, for example, our implementations of strcmp,
strcpy, etc are tolerant to either of the arguments being NULL. However,
calling these functions with a NULL argument is undefined behavior in
the standard. The compiler knows this, and will remove our NULL checks
from the implementation of these functions. We had to disable an
optimization pass to work around this. As gcc was emmitting a warning on
this memset, I would assume we are in a similar situation: gcc has
detected a memset that could work given the current ABI and structure
layout, but that is defined by the standard as "undefined behavior".
This leads the optimizer to a decision of "oh cool, I can simply remove
this and the result will still be what the standard says". It is nice of
gcc to give us a warning when such things happen.


Likewise with unions: the standard defines an "active" part in an union
but leaves tracking which part is active up to the programmer. The
Itanium ABI implements unions by overlapping the various components in
the same RAM space, but this is not mandatory in the standard. If a
compiler ever found a better way to manage unions, they could do so and
still be C++ standard compliant. I can imagine a case, let's say we have
this:

union {
        uint8 a;
        uint8 b[64000];
};

A compiler may manage to track when you only use "a", and decide to not
waste 64K of memory when only one will be used. In a memory constrained
environment, this would be a great feature. If unions were implemented
like this, one could use them to implement std::optional, for example.

Now, this isn't the case in gcc and probably won't be as long as they
follow the Itanium ABI, but still, nothing *in the C++ standard*
prevents this.


As for Haiku, I think it is fine to assume we are going to use the
Itanium ABI, with either clang or gcc, for some time. This means we can
take some shortcuts, but we must be careful about what we do, because we
are still relying on undefined behavior, which the optimizer may start
to exploit as we upgrade gcc versions. Modern versions of gcc provide a
"sanitizer" that will check for such cases at runtime, maybe we should
try to compile our code with this enabled, at least as a learning
experience, and then to fix the most worrying problems identified.

-- 
Adrien.

Other related posts: