Re: CVS commit: src/external/bsd/dhcpcd/dist/src

[Robert Elz <kre%munnari.OZ.AU@localhost>]

    Date:        Sat, 4 Aug 2018 02:15:15 +0200
    From:        Kamil Rytarowski <n54%gmx.com@localhost>
    Message-ID:  <ed8830b0-94d2-fe82-745b-e07fffaf6d58%gmx.com@localhost>


  | In general there shall not be a relation between -O level and
  | sanitizers. Sanitizers do not need -O0  or -g for operation.

That's fine.   But are you doing compiles that way?  (necessary or not)

  | GCC is known for reporting uninitialized variables and I wouldn't blame
  | sanitizers for it.

I wasn't.   The one (which isn't) in the ssh code has been there for
ages, and has compiled successfully, for ages....   There has to be a
reason why it only needed action yesterday.

  | We just initialize them to tune it down and this is the current practice.

Yes, I know - some of them are real potential problems, even if the
circumstances that lead to the problem are so unlikely that we never
see them in real life - others take knowledge about the environment
the compiler does not have, or just require flow analysis more
complex than it is reasonable to expect of the compiler, in order to know
that there is not actually a problem.    The real bugs we fix, obviously.
The ones the compiler cannot detect are not bugs we deal with as you
said.   But when the compiler is able to detect there is no problem, but
we are simply preventing it doing so, we fix the way we use the compiler.

  | GCC also enables more warnings for UBSan that have to be addressed in
  | order to compile the source, as the code would be UB anyway (like
  | changing the signedness bit with a shift operation).

Sure, some of those, even though they're not really problems, are easy
to fix in a totally harmless way.   That's fine.   The UB is a technical C
issue, not really anything that ever fails in those cases though.

There has (is currently) an issue with posix, where a current bug resulution
requires abs(INT_MIN) to be INT_MIN (ie: abs(n) can end up < 0).   In C
that's undefined.   POSIX requires 2's complement however (unlike C) and
can rely upon what happens with 0 - INT_MAX even if C says that is
undefined.    Some people like it, others do not...  (what the end result
will be I have no idea.)

  | I don't agree with strong opinions against cautious warnings/errors from
  | a compiler.

Sure, but when the warning goes off, we need to analyse the issue and
see what the actual problem is, not just blindly do whatever makes the
compiler stop issuing the warning.

  |They are there for purpose and dhcpcd could be really broken
  | with the same code, but with a different context.

No, it could not.   The only possible issue was if the packet was
invalid (too short a len) but that was not what the warning was
about (and could not have been, as there was nothing undefined
if that happened, just an unwanted result).

  | And regarding utility of the Undefined Behavior Sanitizer and coverage
  | of new tests.. we have just caught a bug on pmax that an integer
  | overflow crashed the kernel:

Sure, no-one is saying that the extra work is not worth while.   Just that
you are sometimes fixing non-problems (and causing code churn to do
it - particularly when the code being changed comes from upstream ... in
the dhcpcd case that is not such an issue, as if needed, Roy will fix that
as well, but why would anyone expect the openbsd people to alter ssh
to fix a non problem ?)

Once again, please do not change code to fix gcc warnings unless you
get the same warning with the code compiled with -O2 (or more).

kre