Re: Avoid UB in pslist.h (NULL + 0)
On 24.03.2020 14:30, Kamil Rytarowski wrote: > (3) Patch Clang to start optimizing on NULL + in C so we can return to > points (1) and (2). > I have received a feedback that the particular NULL + 0 issue is intended to be reported to the C committee as a defect. I appreciate this approach. If there is an intention to tune the common interpretation of the C code, it's better to collaborate with the C committee directly. signature.asc Description: OpenPGP digital signature
Re: Avoid UB in pslist.h (NULL + 0)
On 24.03.2020 07:43, Taylor R Campbell wrote: >> Date: Sun, 22 Mar 2020 03:30:56 +0100 >> From: Kamil Rytarowski >> >> On 22.03.2020 01:50, Taylor R Campbell wrote: >>> So far, after several weeks of discussion, nobody has presented a case >>> that there is a credible thread of a compiler actually misbehaving in >>> this scenario. >> >> There are no public declarations (that was requested on a local ML) but >> according to my IRC talks, there were plans to start optimizing on NULL >> + 0 operations, but is was/is considered as a chicken-egg issue. There >> is a time span now to alert users and > > Perhaps it is not a chicken-egg issue, but pointless abuse of leeway > in the standard. So far the public declarations are that Clang > developers realized they _could_ abuse the leeway this way but they > _chose not to_; that essentially everyone involved sees such > `optimization' as abuse; that there are no public reasons stated for > why the C standard diverges from the C++ standard on this note. > > If you have contrary information, please cite it specifically. > I have nothing to add. I'm personally agnostic to both sides. >> Both languages can be synced here as the incompatibility was revealed >> and discussed. Personally I wouldn't be surprised to see adoption of the >> C behavior as nullptr + 0 is invalid in C++ (and it will be likely >> invalid in future C). > > The fragment nullptr + 0 is invalid for an unrelated reason: nullptr > is a pointer to an incomplete type. We've gone over this already. > > It's not helpful to keep bringing it up: at best it's a distraction, > and at worst it gives the impression you might not understand basic > aspects of C and C++ -- an impression which, if true, would make it > all the more important that you _not_ go around tweaking things to > appease sanitizers before discussing and understanding them. > >> So far in reply I get 'just noise from the tool' so maybe my messages >> were not clear enough and reaching authorities (from the clang community >> and C committee) not credible enough. > > You successfully presented a case that it is technically undefined > behaviour. This is not in dispute -- a month ago I cited the specific > place where the C standard neglects to define it[*] -- so I don't > understand why you continue to argue that case. > > [*] https://mail-index.NetBSD.org/tech-kern/2020/02/25/msg026105.html > I reported that these issues are triggered in userland rump (+ in the pslist.h ATF tests). Rump has userland assumptions with compiler flags. I already declarared that using assembly code is not planned to be touched. >>> (b) Change how we invoke ubsan and the compiler by passing >>> -fno-delete-null-pointer-checks to clang. joerg objected to this >>> but I don't recall the details off the top of my head; joerg, can >>> you expand on your argument against this, and which alternative >>> you would prefer? >> >> I tried to enable -fno-delete-null-pointer-check for RUMPKERNEL, but I >> was asked to revert... after first getting acknowledge from you that it >> is a good idea... > > This is an inaccurate representation of what happened. What I said is > (https://mail-index.NetBSD.org/tech-kern/2020/03/08/msg026125.html): > > `Adding -fno-delete-null-pointer-checks for clang too sounds >sensible to me in general, but please check with joerg first. It >remains unclear to me that it's necessary here.' > > I asked you to revert because: > (a) the discussion was still ongoing without a conclusion, > (b) it remained unclear whether the change was necessary, I noted that it is necessary at least for memcpy(NULL, NULL, X)-like usage in rump. Even if we ignore NULL + 0. The kernel code is already prebuilt with -fno-delete-null-pointer-checks, but not userland rump. This causes slight incompatibilities. I don't see what's unclear. I k > (c) you failed to check with joerg like I asked, and I checked earlier that joerg disagrees with GCC and he has its interpretation of the standard that memcpy(NULL, NULL, 0)-like optimization is wrong. > (d) joerg objected. > And GCC developers do not support this (me neither). > I appreciate that sometimes it takes longer than you or I might like > to get a clear explanation out of joerg, but it is also fatiguing to > have to correct misrepresentations of positions in long meandering > threads in order to ensure changes you are itching to make -- or have > already made despite ongoing discussion -- are justified and correct. > >>> (c) Change how we invoke ubsan, but just ubsan -- not the compiler. >> >> UBSan is an integral part of a compiler. > > What I meant is: Change the options we pass for builds with the > sanitizer enabled, but not the options we pass for normal builds. In > other words, either pass _both_ -fsanitize=undefined and > -fno-delete-null-pointer-checks (if MKSANITIZER=yes), or _neither_ of > them (if MKSANITIZER=no). > This could be integrated into
Re: Avoid UB in pslist.h (NULL + 0)
> Date: Sun, 22 Mar 2020 03:30:56 +0100 > From: Kamil Rytarowski > > On 22.03.2020 01:50, Taylor R Campbell wrote: > > So far, after several weeks of discussion, nobody has presented a case > > that there is a credible thread of a compiler actually misbehaving in > > this scenario. > > There are no public declarations (that was requested on a local ML) but > according to my IRC talks, there were plans to start optimizing on NULL > + 0 operations, but is was/is considered as a chicken-egg issue. There > is a time span now to alert users and Perhaps it is not a chicken-egg issue, but pointless abuse of leeway in the standard. So far the public declarations are that Clang developers realized they _could_ abuse the leeway this way but they _chose not to_; that essentially everyone involved sees such `optimization' as abuse; that there are no public reasons stated for why the C standard diverges from the C++ standard on this note. If you have contrary information, please cite it specifically. > Both languages can be synced here as the incompatibility was revealed > and discussed. Personally I wouldn't be surprised to see adoption of the > C behavior as nullptr + 0 is invalid in C++ (and it will be likely > invalid in future C). The fragment nullptr + 0 is invalid for an unrelated reason: nullptr is a pointer to an incomplete type. We've gone over this already. It's not helpful to keep bringing it up: at best it's a distraction, and at worst it gives the impression you might not understand basic aspects of C and C++ -- an impression which, if true, would make it all the more important that you _not_ go around tweaking things to appease sanitizers before discussing and understanding them. > So far in reply I get 'just noise from the tool' so maybe my messages > were not clear enough and reaching authorities (from the clang community > and C committee) not credible enough. You successfully presented a case that it is technically undefined behaviour. This is not in dispute -- a month ago I cited the specific place where the C standard neglects to define it[*] -- so I don't understand why you continue to argue that case. Inline asm is technically undefined by the standard too but it would be worse than useless if ubsan warned about every instance of that. [*] https://mail-index.NetBSD.org/tech-kern/2020/02/25/msg026105.html > > (b) Change how we invoke ubsan and the compiler by passing > > -fno-delete-null-pointer-checks to clang. joerg objected to this > > but I don't recall the details off the top of my head; joerg, can > > you expand on your argument against this, and which alternative > > you would prefer? > > I tried to enable -fno-delete-null-pointer-check for RUMPKERNEL, but I > was asked to revert... after first getting acknowledge from you that it > is a good idea... This is an inaccurate representation of what happened. What I said is (https://mail-index.NetBSD.org/tech-kern/2020/03/08/msg026125.html): `Adding -fno-delete-null-pointer-checks for clang too sounds sensible to me in general, but please check with joerg first. It remains unclear to me that it's necessary here.' I asked you to revert because: (a) the discussion was still ongoing without a conclusion, (b) it remained unclear whether the change was necessary, (c) you failed to check with joerg like I asked, and (d) joerg objected. I appreciate that sometimes it takes longer than you or I might like to get a clear explanation out of joerg, but it is also fatiguing to have to correct misrepresentations of positions in long meandering threads in order to ensure changes you are itching to make -- or have already made despite ongoing discussion -- are justified and correct. > > (c) Change how we invoke ubsan, but just ubsan -- not the compiler. > > UBSan is an integral part of a compiler. What I meant is: Change the options we pass for builds with the sanitizer enabled, but not the options we pass for normal builds. In other words, either pass _both_ -fsanitize=undefined and -fno-delete-null-pointer-checks (if MKSANITIZER=yes), or _neither_ of them (if MKSANITIZER=no). > If we introduce code that relies on UB assumptions it is rather opposite > of comprehensible code. Handling UB imposes handling corner-cases that > is rather a good style of programming. The kernel relies on UB assumptions everywhere, because it's a _part_ of the C implementation -- large swaths of the kernel are responsible for driving a physical machine to implement the C abstract machine. As the maintainers of the kernel, we have to distinguish the UB that actually may have bad consequences, not blindly reject all UB because the letter of the standard doesn't define it in the C abstract machine. > > (f) Ditch ubsan. Does it have enough _true_ alarms, detecting actual > > bugs, to make it worth keeping, or are we wasting a lot of time to > > appease a tool that isn't actually giving us much value? > > 1. Signed
