Issue metadata
Sign in to add a comment
|
clang: Suppress optimization of NULL pointer check for struct members |
||||||||||||||||||||||||
Issue descriptionFrom https://groups.google.com/a/google.com/forum/?utm_medium=email&utm_source=footer#!msg/c-toolchain-team/B3quWYKCSsk/Vtf-AVyjBQAJ : For quite a long time we've been stuck with an UB in the Linux kernel caused by the following code: #define llist_for_each_entry_safe(pos, n, node, member) \ for (pos = llist_entry((node), typeof(*pos), member); \ &pos->member != NULL && \ (n = llist_entry(pos->member.next, typeof(*n), member), true); \ pos = n) (taken from http://elixir.free-electrons.com/linux/latest/source/include/linux/llist.h#L149) Basically Clang assumes &pos->member can never be NULL and optimizes the loop condition away. While this is a legit transformation from the C Standard point of view, this is certainly not what the Linux kernel developers are expecting from the compiler. In fact when |node| is NULL, the value of |pos| becomes equal to -offsetof(typeof(*pos), member), so pos->member also becomes NULL. Since many kernel teams at Google are going to switch to Clang soon, we need to work around this problem somehow. IIUC the optimization in question is suppressed in GCC by either -fno-delete-null-pointer-checks or -fno-strict-overflow, but that doesn't work in Clang (the former flag isn't supported at all). LLVM bug: https://bugs.llvm.org/show_bug.cgi?id=9251 niravd@ volunteered to take a look at the implementation. For CrOS this is mainly a tracking bug (for now). An upstream patch for drm/i915 (1eb13b03a49c "UPSTREAM: drm/i915: Move object release to a freelist + worker") that was recently integrated in our v4.4 tree uses llist_for_each_entry_safe, which causes a clang built kernel to crash unless a workaround/hack is used.
,
Jul 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/88223e75acc227cbc631355c0ff8f6a1a0f7d0bb commit 88223e75acc227cbc631355c0ff8f6a1a0f7d0bb Author: Bernhard Rosenkränzer <bero@linaro.org> Date: Sat Jul 15 01:01:17 2017 HACK: llist: Fix for compilation with clang The clang compiler treats the memory address of a structure field as tautologically true when compared to non-NULL. This breaks the macros in llist.h when compiled using clang. Fix this by not comparing the address of a structure field to NULL when determining the end of the llist. Refer to -Wtautological-pointer-compare in clang for more information. llist is used in very few places in the kernel. Recently a bunch of drm/i915 changes from upstream were integrated, one of them (1eb13b03a49c "UPSTREAM: drm/i915: Move object release to a freelist + worker") uses llist_for_each_entry_safe, which causes a clang built kernel to crash unless a hack like this is in place. A proper fix should be implemented in clang. Use this hack until the fix is available and revert it later. BUG= chromium:742607 , chromium:702741 TEST=build for squawks with clang and boot Signed-off-by: Hans Petter Selasky <hps@selasky.org> Signed-off-by: Bernhard Rosenkrnzer <bero@linaro.org> (cherry picked from android-git.linaro.org kernel/hikey-clang android-hikey-linaro-4.9-clang commit 4f3c3c1e7b153e333603be74d786d79bb872e8ff) Change-Id: I74eb55a2d1acf11cc209f0b91a1b85fa49eaadb7 Signed-off-by: Matthias Kaehlcke <mka@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/571403 Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/88223e75acc227cbc631355c0ff8f6a1a0f7d0bb/include/linux/llist.h
,
Jul 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/eff216dcdc22e5e784d8ad75d3ee784995be9854 commit eff216dcdc22e5e784d8ad75d3ee784995be9854 Author: Matthias Kaehlcke <mka@chromium.org> Date: Fri Jul 28 19:28:02 2017 Revert "HACK: llist: Fix for compilation with clang" Use the recently merged upstream commit beaec533fc27 ("llist: clang: introduce member_address_is_nonnull()") instead. This reverts commit 88223e75acc227cbc631355c0ff8f6a1a0f7d0bb. BUG= chromium:742607 , chromium:702741 TEST=none Change-Id: Ia8d8c84c4836b58fd4fcfad4909a795b88ec20be Reviewed-on: https://chromium-review.googlesource.com/590654 Commit-Ready: Matthias Kaehlcke <mka@chromium.org> Tested-by: Matthias Kaehlcke <mka@chromium.org> Reviewed-by: Douglas Anderson <dianders@chromium.org> [modify] https://crrev.com/eff216dcdc22e5e784d8ad75d3ee784995be9854/include/linux/llist.h
,
Jul 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/5c12331d8d0e90d1df8b434d11beab7af35950e0 commit 5c12331d8d0e90d1df8b434d11beab7af35950e0 Author: Alexander Potapenko <glider@google.com> Date: Fri Jul 28 19:28:04 2017 UPSTREAM: llist: clang: introduce member_address_is_nonnull() Currently llist_for_each_entry() and llist_for_each_entry_safe() iterate until &pos->member != NULL. But when building the kernel with Clang, the compiler assumes &pos->member cannot be NULL if the member's offset is greater than 0 (which would be equivalent to the object being non-contiguous in memory). Therefore the loop condition is always true, and the loops become infinite. To work around this, introduce the member_address_is_nonnull() macro, which casts object pointer to uintptr_t, thus letting the member pointer to be NULL. BUG= chromium:742607 , chromium:702741 TEST=build for squawks with clang and boot (cherry picked from commit beaec533fc2701a28a4d667f67c9f59c6e4e0d13) Change-Id: Ia3df377f46611527a9ecb65fbaff87902a5ae216 Signed-off-by: Alexander Potapenko <glider@google.com> Tested-by: Sodagudi Prasad <psodagud@codeaurora.org> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Matthias Kaehlcke <mka@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/590655 Reviewed-by: Douglas Anderson <dianders@chromium.org> [modify] https://crrev.com/5c12331d8d0e90d1df8b434d11beab7af35950e0/include/linux/llist.h
,
Sep 25 2017
mka@ Do you still need this?
,
Sep 27 2017
glider@ submitted a patch to address the original issue (see #4), there is currently no other known instance. However I wouldn't be surprised if other kernel code still relies on -fno-delete-null-pointer-checks and this bites us again. I think it would be good to add support for -fno-delete-null-pointer-checks to clang if the effort is limited.
,
Sep 28 2017
I believe niravd@ was supposed to implement it. So it probably makes more sense to file a buganizer bug and assign to him.
,
Dec 26 2017
,
Jan 2 2018
[It appears that a bunch of old cros issues bulk-added the "Infra" component recently, but they should probably be "Infra>Client>ChromeOS".]
,
Jan 4 2018
,
Feb 5 2018
,
Jul 10
|
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by mka@chromium.org
, Jul 13 2017