zstd: ifdef out visibility of floats in kernel for gcc
AbandonedPublic
Actions

Authored by rlibby on Dec 13 2018, 7:08 AM.

Details

Reviewers

cem

Summary

Minimal work-around for upstream issue:
https://github.com/facebook/zstd/issues/1386

Test Plan

make CROSS_TOOLCHAIN=amd64-gcc TARGET=amd64 buildkernel

Diff Detail

Lint

No Lint Coverage

Unit

No Test Coverage

Build Status

Buildable 21533
Build 20841: arc lint + arc unit

Event Timeline

rlibby created this revision.Dec 13 2018, 7:08 AM

Harbormaster completed remote builds in B21533: Diff 51949.Dec 13 2018, 7:09 AM

@cem is direct commit to sys/contrib/zstd okay for this?

I'm honestly not sure if direct commits to vendored code are ok. IMO, go for it, but you might consult with someone more senior (@jhb ?).

The functional change looks fine to me — I'm assuming you've compile-tested this with GCC, since that was the motivation for the change (and the other changes you've proposed on $WORK Slack).

sys/contrib/zstd/lib/compress/zstd_compress_internal.h
692	One nitpicky suggestion is that usually `#if` conditions are not parenthesized and also the usual style about spaces between operands and operator applies. (If it's the prevailing style in the file, ignore this nitpick.)

This revision is now accepted and ready to land.Dec 13 2018, 5:42 PM

Sorry, one other thing occurred to me.

sys/contrib/zstd/lib/compress/zstd_compress_internal.h
692	Actually, one other request for you. Please make these conditions: `#if DEBUGLEVEL >= 2 && defined(_KERNEL)` (Or whatever equivalent construction is stylistically appropriate for the file.) We don't want to break debug in userspace, where floats more are appropriate.

This revision now requires changes to proceed.Dec 13 2018, 5:45 PM

rlibby added inline comments.Dec 13 2018, 5:54 PM

sys/contrib/zstd/lib/compress/zstd_compress_internal.h
692	Sure, I can make those changes. What I did here was copy the ifdef guard around `DEBUGLOG` in sys/contrib/zstd/lib/common/debug.h, i.e. this is the same condition that guards definition of `DEBUGLOG` in the first place. So I don't think it will break anything. But being more restrictive for the kernel seems harmless. Incidentally, `DEBUGLOG` would not work in the kernel as coded anyway (`fprintf(stderr, ...)`).

cem accepted this revision.Dec 13 2018, 5:59 PM

cem added inline comments.

sys/contrib/zstd/lib/compress/zstd_compress_internal.h
692	Oh, in that case, please ignore my _KERNEL suggestion. When you say copy the ifdef guard from debug.h, you mean the style came from upstream as well? If so, please ignore my style suggestions too.

This revision is now accepted and ready to land.Dec 13 2018, 5:59 PM

rlibby added inline comments.Dec 13 2018, 9:56 PM