Messages in this thread | | | From | Nick Terrell <> | Subject | Re: [PATCH 2/2] zstd: Backport Huffman speed improvement from upstream | Date | Wed, 22 Nov 2023 01:09:26 +0000 |
| |
> On Nov 21, 2023, at 3:54 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Tue, 21 Nov 2023 at 12:35, Nick Terrell <terrelln@meta.com> wrote: >>> >>> Honestly, any coding rule that includes "don't use the do-while-zero >>> construct" is actively broken shit. >>> >>> Please just fix your upstream rules. Because they are incredible garbage. >> >> Yeah, that’s the plan. Visual Studios fixed that compiler bug in VS2015 [0], >> so we should be safe to migrate to safer macros. > > I don't even use MSVS, but a minute of googling shows that you should > never have done that silly "avoid sane C", and you should always just > have done > > #pragma warning (disable: 4127) > > for MSVC. > > Honestly, the fact that the result was instead to disable that > standard - and required - construct in the project makes me worry > about the whole zstd thing. WTF?
Admittedly our coding guidelines are overly conservative. And here we are updating to our macros to use the do { } while (0) construct in this PR [0].
However, we are also very conservative in our testing. We have very extensive coverage-guided fuzz testing running continuously for safety of (de)compressing untrusted data, round-trip correctness, and more.
We take security & correctness very seriously. If you have any questions I’d be happy to answer them, and I should collect our testing process publicly in one place, so we can reference that.
If you have any further suggestions I’m very open to them, and I am grateful for the time you’re taking to improve zstd.
[0] https://github.com/facebook/zstd/pull/3831
> The do-while-zero construct is _so_ important that there are (sane) > projects that literally *require* the use of it. See for example MISRA > code safety rules. > > The kernel rules aren't quite that strict, but yes, do-while-zero is > very much "you should *absolutely* do this" along with all the usual > "make sure you have parentheses around macro arguments" rules. > > We had some RFC patches for this area: > > https://lore.kernel.org/all/20230511152951.1970870-1-mathieu.desnoyers@efficios.com/
Agreed.
> And on that note, when I googled for the solution to the MSVC brain > damage, I was distressed by how many hits I saw where people thought > the do-while-zero pattern was some "legacy pattern". > > That just shows that there are lots of incompetent people simply do > not understand why it's actually *required* for reliable parsing of > macros. This is not some "historical stylistic" issue, it's literally > a correctness issue for generic macro usage. > > Linus
| |