[coreboot] [RFC] Fix undefined behavior with left shifts in whole code base

[ron minnich]

I refer you to the following code:
https://github.com/hephaex/unix-v6/blob/daa355109625a50e6b1080184dee30c9136549d1/param.h#L72

It's this:
/*
 * structure to access an
 * integer in bytes
 */
struct
{
char lobyte;
char hibyte;
};

What's the point? The point is code linke this:
lpr = (tp->t_speeds.hibyte<<10) ...

note how we're treating t_speeds like it's that anon struct above. how's
that happen? What's t_speeds?
int t_speeds; /* output+input line speed */

Wait, what? I could just take an int variable, then use an anon struct to
break it into bits? Yes, I could. This is C, 1976.

The point is, C changes over time, and so does how you use it. At some
point people realized this model was kind of dangerous, and over the
objections of many people, the inconvenience of type checking came in. The
changes in C are so pervasive that when Dennis checked, years later, the
original C compilers ... were no longer C. They would not compile.

The rule over the years is that you should always err in favor of compiler
level checking of your code, even when it adds lots of inconvenience, as
people make mistakes.

So I would actually be in favor of what paul is advocating, but not the
inconistency. To keep it consistent, I don't see that just using 1U
everywhere is that huge a deal. C is not that pretty any more, so this
little bit of ugliness doesn't strike me as a big deal. Bugs from stuff
like 3<<31 scare me more.

Just my $.02

ron

On Fri, Feb 16, 2018 at 2:38 PM Julius Werner <jwerner at chromium.org> wrote:

> I’d really like to *enable as much warnings* as possible to let the
>> build tools *catch as much errors as possible*, and having to adapt the
>> code to work with this is in my opinion worth the effort.
>>
>
> This is not just a question of whether we want to spend the one-time
> effort to adapt the code. It will be a constant readability issue going
> forward. Almost no project out there writes shifts like that and almost no
> programmer is used to reading them like that. I agree that automated
> sanitizer coverage is a very good thing, but it is not the only thing that
> matters. If a sanitizer tool forces us two rewrite code in a way that
> creates serious friction for day-to-day development, then maybe we need to
> wait until the authors of that tool add options to make it less aggressive
> in that regard. GCC has already recognized the insanity of reporting that
> construct as an error every time and thus given us the -Wshift-overflow=1 /
> -Wshift-overflow=2 distinction, so I hope the UBSAN guys may eventually get
> there as well.
>
> For now, I think we should deactivate the whole shift test (I'm not an
> UBSAN expert but from the documentation it sounds like
> -fno-sanitize=shift-base should work), and then we'll still get the vast
> majority of coverage without imposing excessive hoops to jump through onto
> developers.
> --
> coreboot mailing list: coreboot at coreboot.org
> https://mail.coreboot.org/mailman/listinfo/coreboot
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot/attachments/20180403/31da5780/attachment.html>