linguini1 commented on PR #17300: URL: https://github.com/apache/nuttx/pull/17300#issuecomment-3517275761
I'm going to be honest, the discussion on this PR has left me a little confused as well. First, I agree that not all architectures cause NULL pointer exceptions on de-reference. In fact, I'm fairly certain that C states this is undefined behaviour and doesn't make any guarantees about exceptions. Therefore, I agree that they have value. Second, I am confused by the change. I know that in many places in NuttX, we have `DEBUGASSERT(ptr != NULL)` as a formality for developers. I.e., the function takes some `ptr` that the user passed, so there are no guarantees that it's non-null. However, if the function is only called from kernel space, then the "caller" is a NuttX developer. So we make the check a DEBUGASSERT rather than a run-time check (i.e. `if (ptr == NULL) // do something `) so that when production code turns off debug features, the check can be ignored. And that is because we trust NuttX developers to have made a guarantee that they're not passing invalid pointers to kernel functions after they're done debugging/testing their feature/change. This scenario makes sense to me. The scenario that does not make sense to me is the one in this PR. The same approach seems to be taken, but it is mentioned in the code itself that "rtcb may be NULL only during early boot-up phases". If this is true, then the rest of the function (without the run-time check and DEBUGASSERT disabled) is undefined behaviour. It will be doing operations on a NULL pointer. In this case, `rtcb` being NULL isn't some developer mistake, but it is something that is guaranteed to happen at some point early in the boot process. It needs a run-time check for that reason. I may not be understanding correctly, but if that is the case then I agree that this commit needs to be reverted. @xiaoxiang781216 @hujun260, could you explain why the `rtcb` pointer can never be null in `sched_lock`? How is that guaranteed? Is it because `sched_lock` is never called in the early-boot phase? @anchao I agree with you on the merge of this PR. I think it's important that we respect other reviewers. If they disagree, it is our job to convince them, and if we cannot convince them, we take the PR merge to a mailing list vote. Especially a PR like this one should not be merged until reviewers requesting changes are satisfied. Why? * It affects all NuttX platforms since it is in the scheduler, which is an important piece of code * It is a very small change with a pretty minor justification. Why do we need to remove this redundant check? Probably for more speed in hot paths, but there's no quantification of how much speed we're gaining or any indication if the old version was causing problems for a user. This PR is optimization for the sake of optimization, which is fine, but I don't understand the urgency of merging it. @xiaoxiang781216 I do understand that you explained your point of view to @anchao, and your comments about why we must perform NULL checks in NuttX make sense (not all systems have an MMU/MPU). I am still a little confused about how it is guaranteed that `rtcb` cannot be NULL, and I imagine that anchao is as well from their comments. However, this is the second time you've dismissed a reviewer's change request and merged the PR right afterwards. It is not fair to other reviewers to do that, their change requests must be self-dismissed to show that they have been convinced. It is very frustrating for the reviewer and makes it feel like you did not have respect for their review. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
