Separate the first iteration of the loop in RB_INSERT_COLOR from the rest. That iteration always reaches the 'continue' statement, and gcc compilers, not knowing that, issue warnings about the use of an uninitialized 'child' variable. By separating the first iteration, the always-true test is not made, saving one instruction, but at the cost of code size growing slightly.
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
Single instruction to clear a register is basically free on modern architectures. All it does it marks the register as having zero (sometimes not writing actual zero) and eliminating dependencies on previous register value. On the other hand, adding more code means that we consume more space in the icache, there are more instructions to parse etc.
I think that the net effect of either approaches (original with explicit NULL and this one) cannot be measured, but I would think that less code the better both for humans and CPUs, in this case.
Comment Actions
I'll defer to you and kib as maintainers whether this is a good direction. Personally I do like the flattening of the structure that results from moving line 561/563 and following out of the loop, but I'm less sure about the initial loop unrolling. FWIW my preference for the warning would still be to just NULL initialize child.
Would you take a look at this patch and see if you think there's anything useful in this formulation? I was working on a similar way to try to make both compilers happy but I didn't get there. The patch in the link quiets the gcc warning and makes the gcc code shorter, but makes the clang code longer, clang apparently starts pushing registers onto the stack.
https://github.com/rlibby/freebsd/commit/2597adf5d05eedeacb283356920da7f3765d7f8f
Also, I'll go ahead and push D52939 which this would conflict with, unless you want me to wait on it? I was hoping to push D52939 and D52940 together just to minimize build churn for people, but now it seems more useful to push to resolve the conflicts.
| sys/sys/tree.h | ||
|---|---|---|
| 552–559 | I don't understand the change in order between what was before line 555 if ((_RB_BITS(gpar) & _RB_LR) == 0) and only if that is true then climbing up a level in the tree, and now in the patch climbing up a level before what I understand to be the new equivalent test on new line 556. Am I reading this wrong? | |
| sys/sys/tree.h | ||
|---|---|---|
| 552–559 | Before, if we were to climb one level up the tree and return NULL, we would flip a bit in parent (549), set child (557), set elm (558), set parent to gpar (623) and stop iterating if gpar was NULL. Of course, if gpar was NULL, we needn't have bother with setting child or elm or parent. Here, if we were to climb one level up the tree and return NULL, we would flip a bit in parent (548) and return if gpar was NULL, without affecting those other variables. I don't see how this affects the correctness of anything. Is there some case where it would? | |
Comment Actions
Despite your best efforts, I still cannot do a gcc build. I installed gcc-14 as you suggested, but my buildkernel fails with a mysterious (to me) message about being unable to read an object file. So I can't test for myself what this does to the gcc build.
Also, I'll go ahead and push D52939 which this would conflict with, unless you want me to wait on it? I was hoping to push D52939 and D52940 together just to minimize build churn for people, but now it seems more useful to push to resolve the conflicts.
I have no objection to your pushing D52939.