Hi, On 21/10/2020 20:41, Jan Kiszka wrote: > On 21.10.20 16:53, Andrea Bastoni wrote: >> As part of a project I was experimenting with compiler warning options >> that partial covers coding standard rules from e.g., MISRA 2012. An >> example is Wconversion, and the narrower sign-conversion, sign-compare, >> pointer-arch, cast-qual. E.g., Wconversion covers MISRA 2012 10.3 (and >> parially 10.4, 10.1). >> > > Thanks a lot, this is valuable work! While standard compliance does not give > you > good code per se (*) and - if blindly followed - can even be > counterproductive,
Agreed. It's not uncommon to actually indroduce bugs while blindly fixing warnings... I hope I didn't do it ;) > some warnings are low-hanging fruits, some report missing cleanups, and there > was at least one bug caught this way. I'll go through the patches carefully > and > provide further feedback later. Thanks! > > (*) I once heard the saying that most coding standards exist in order to > reduce > the damage unexperienced coders can cause to critical programs. The sad truth > is > that such weird combinations are not uncommon in commercial software > projects... > >> Before enabling those, I've enabled some other warnings to filter out >> definition/declaration "issues" that would clutter the compilation >> otherwise. Specifically (Patch 0001), I've added: -Wextra -Wundef >> -Wnested-externs -Wshadow -Wredundant-decls -Wdeprecated >> >> The warnings were not too many (most of them related to >> unused-parameters), but there were some interesting ones e.g.: >> >> hypervisor: provide dedicated declaration for __page_pool and __text_start in >> globals.h > > Actually, it's only __page_pool, and we should make that global via paging.h. Ah, right, __text_start is only used in setup. >> hypervisor: introduce uptr_t (depending on the wordsize) and define size_t >> accordingly >> hypervisor, arm-common: provide an explicit uint INVALID_CPU_ID >> >> >> The patch series fixes the warnings generated by "-Wextra -Wundef >> -Wnested-externs -Wshadow -Wredundant-decls -Wdeprecated", and two small >> bugs: >> >> x86: bitops: only x86_64 is supported, avoid picking the wrong default suffix > > How did you come across that one? Luckily, we only ever set bit 0... Saw this warning and took a look at the disassembly. Panic is probably not so common anyway. hypervisor/arch/x86/include/asm/bitops.h:54: Warning: no instruction mnemonic suffix given and no register operands; using default for `bts' >> arm64: smmu: fix double negative > > Good catch. I will fold that into the original commit, providing credits. Thanks. >> >> Up to "0024 x86: bitops: only x86_64 is supported, avoid picking the >> wrong default suffix", the series is the same for both master and next. >> >> These two >> >> arm64: smmu: Wsign-compare: make iterator uint >> arm64: smmu: fix double negative >> >> are only next related. >> >> I've compile-checked x86, arm, arm64, and tested arm64 (ZCU102), but I don't >> have a suitable arm / x86 physical targets and I'm still figuring out >> a crash I have on qemu-x86 (can reproduce with plain jailhouse-image >> qemu image). > > Obviously, I would be interested in learning about how that issue looks like > and > how you reproduce it. Sorry. It's just me. I redirected the console not to use the VC but the serial and I must have changed the default configuration. The exact start-qemu.sh works. >> >> I don't know if there's interest in extending the compiler flags with >> warnings about code structure, but since the code compiles cleanly with >> the patches, I thought about sharing them. > > In general, anything that is providing a reasonable chance to reveal real > issues > early is welcome. But it's always a balance between excessive code > restructuring > to avoid false-positives or still oversensitive compilers and potentially > valuable warnings. > >> >> Additional info: >> gcc (Debian 10.2.0-13) 10.2.0 >> >> JFTR: >> Wcast-qual generates 276 warnings on arm64 (but they looks ~ the same, >> it could be feasible fixing them) Wconversion + Wpointer-arith are a >> worse beast (~ 1400 occurrences), and the "int" return convention in >> Linux doesn't make it easy to cleanly fix them. >> > > Those fall under > > "W=3 - more obscure warnings, can most likely be ignored" > (linux/scripts/Makefile.extrawarn) > > and there are likely good reasons for that. Eheh, maybe too many warnings ;) Maybe for smaller safety oriented code bases some of them could be considered. E.g., it should be doable to preserve a const qualifier, and a (void *) may be just casted to the right type when used in an operation, or if security is of interest Wpadded could help for the API interface. But yes, Wconversion, is a lot for likely very few catches... -- Thanks, Andrea Bastoni -- You received this message because you are subscribed to the Google Groups "Jailhouse" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/jailhouse-dev/aec791f3-9637-b08e-99b5-56d2b6341cce%40tum.de.
