Date: Sat, 4 Aug 2018 02:15:15 +0200 From: Kamil Rytarowski <n...@gmx.com> Message-ID: <ed8830b0-94d2-fe82-745b-e07fffaf6...@gmx.com>
| 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