5 ms·

My experience with a lot of the UB-is-bad crowd is that they don't have much of an appreciation for semantics in general. That is to say, they tend to react to

My experience with a lot of the UB-is-bad crowd is that they don't have much of an appreciation for semantics in general. That is to say, they tend to react to particular compiler behavior, and they don't have any suggestions for how to rectify the situation in a way that preserves consistent semantics. And when you try to pin down the semantics, you usually end up with a compiler that has to "do what I mean, not what I said." A salient example that people often try on me, that I don't find persuasive, is the null pointer check: void foo(int *x) { int val = *x; if (!x) return; /* do real stuff */ } "The compiler is stupid for eliminating that null check!" "So you want the code to crash because of a null pointer dereference then." "No, no, no! Do the check first, and dereference only when it's not null!" "That's not what you said..."

mzs 5y ago
On a lot of systems load from address zero doesn't segfault, I'm fine with the cpu loading it. I'm disappointed that the new compiler removed the check someone wrote a dozen years ago to prevent the cpu from overwriting something important though.
- jcranmer 5y ago
  In the case where loads from address zero are legal, the dereference-means-non-null-pointer assumption is disabled.
  - mzs 5y ago
    Seems it wasn't disabled in 2009 https://github.com/torvalds/linux/commit/a3ca86aea507904148870946d599e07a340b39bf https://github.com/torvalds/linux/commit/a3ca86aea5079041488...
    - jcranmer 5y ago
      That case wasn't one of the cases where null pointers are valid memory, judging from the commit message. (There was a case where gcc had a bug where it didn't disable that check properly, but this doesn't appear to be that case.)
- gpderetta 5y ago
  The null pointer need not map to address zero exactly to cater to this sort of corner cases.
bewaretheirs 5y ago
Actually, yes, I do want that code to crash if x is NULL.
- pcwalton 5y ago
  Then you should logically be OK with deleting the null pointer check, as that check is unreachable.
  - bewaretheirs 5y ago
    The compiler can't know if page 0 is mapped.
  - mpweiher 5y ago
    Nope.
  - nkurz 5y ago
    Are you familiar with this classic example? http://blog.llvm.org/2011/05/what-every-c-programmer-should-know_14.html http://blog.llvm.org/2011/05/what-every-c-programmer-should-... The problem (if I understand it right) is that "Redundant Null Check Elimination" might be run first, and will get rid of the safety return. But then the "Dead Code Elimination" can be run, which gets rid of the unused read, and thus removes the crash. Which means that rather than being logically equivalent, you can end up with a situation where /* do real stuff */ (aka /* launch missiles */) can be run despite the programmer's clear intentions.
    - Dylan16807 5y ago
      Right, each optimization is fine on its own but the combination is dangerous.
  - saagarjha 5y ago
    Not really. My understanding of the opinion (which I don't share, FWIW, and you probably know this) is that the null dereference should not be deleted, and it should be marked as an undeleteable trap similar to what an out-of-bounds access might be in Rust. That is, not unreachable in the sense of "code can never reach here", but unreachable in the sense that if it is hit then no other code executes.
nkurz 5y ago
> "No, no, no! Do the check first, and dereference only when it's not null!" I don't think I've heard anyone express this preference. If they did, I'd agree with you that they are being unreasonable. My impression is that practically everyone on the "portable assembler" team would think it's perfectly reasonable to attempt the read, take the SIGSEGV if x is NULL, and crash if it's not handled. Most would also be happy skipping the read if the value can be shown to be unused. None would be happy with skipping the conditional return. Where it gets thornier is when there is "time travel" involved. What if the unprotected read occurs later, and instead of just returning on NULL, we want to log the error and fall through: void foo(int *x) { if (!x) /* log error */; int val = *x; return; } Is it reasonable to have the later unconditional read of x cause the compiler to skip the earlier check of whether x is NULL? In the absence of an exit() or return in the error handler, I think it would be legal for the compiler to skip both the error logging and the unused load and silently return, but I don't want the compiler to do this. I want it to log the error I asked it to (and then optionally crash) so I can identify the problem. Alternatively, I'd probably be happy with some compile time warning that tells me what I did wrong.
- nlewycky 5y ago
  How do you feel about: void foo1(int *p) { *p = 7; printf("%d\n", *p); free(p); } May the compiler replace that with puts("7") and free()? Recall that free() is a no-op when the pointer is NULL. Are you arguing that each function may reduce pointer accesses down to one but not down to zero? What about int foo2() { int *p = malloc(400); *p = 0; /* ... code here was inlined then optimized away ... */ ret = 7; free(p); return ret; } ? May we remove '*p = 0;', whether we remove the malloc+free or not? Generally speaking the compiler tries not to reason about the whole function but just to look at the smallest possible collection of instructions, like how add(x, x) can be pattern matched to mul(x, 2) and so on. Having to reduce to one but not zero memory accesses per function is not easily compatible with that model. We would have to model branches that make the access conditional, the length of accesses may differ (what if 'p' is cast to char* and loaded), read vs. write, multiple accesses with different alignment, and maybe other things I haven't considered. Both gcc and clang provide -fsanitize=null which checks all pointer accesses for null-ness before performing them. These can be surprising, your code (libraries you didn't write, headers you included) may be dereferencing NULL and relying on the optimizer to remove the invalid access. IMO there should be a "pointer is null-checked after use", it's a clear example where the programmer wrote something other than what they intended.
jwatte 5y ago
I would expect the code to crash when passed a null pointer, absolutely! And the if statement is there to let me recover after using "continue" in the debugger. And if I run on an ISA without protected memory, that load will not actually crash, and I'm OK with that. That's a level of UB differing-behavior (more like ID, really) that's reasonable. I know of no experienced C programmer who would expect the compiler to re-order the statements. That sounds very much like a strawman argument to me! I'd also be OK with a compiler that emitted a warning: "This comparison looks dumb because the rules say it should do nothing." Emitting that warning is helpful to me, the programmer, to understand where I may have made a mistake. Silently hiding the mistake and cutting out parts of code I wrote, is not generally helpful, even if there exist some benchmark where some inner loop will gain 0.01% of performance if you do so. After all: The end goal is to produce and operate programs that run reliably and robustly with good performance, with minimum programmer cost. Saying that the possible performance improvements of code that nobody will run because it's buggy absolutely trump every other concern in software development, would be a statement I don't think reflects the needs of the software development profession.

mzs 5y ago

On a lot of systems load from address zero doesn't segfault, I'm fine with the cpu loading it. I'm disappointed that the new compiler removed the check someone wrote a dozen years ago to prevent the cpu from overwriting something important though.

jcranmer 5y ago
In the case where loads from address zero are legal, the dereference-means-non-null-pointer assumption is disabled.
- mzs 5y ago
  Seems it wasn't disabled in 2009 https://github.com/torvalds/linux/commit/a3ca86aea507904148870946d599e07a340b39bf https://github.com/torvalds/linux/commit/a3ca86aea5079041488...
  - jcranmer 5y ago
    That case wasn't one of the cases where null pointers are valid memory, judging from the commit message. (There was a case where gcc had a bug where it didn't disable that check properly, but this doesn't appear to be that case.)
gpderetta 5y ago
The null pointer need not map to address zero exactly to cater to this sort of corner cases.

bewaretheirs 5y ago

Actually, yes, I do want that code to crash if x is NULL.

pcwalton 5y ago
Then you should logically be OK with deleting the null pointer check, as that check is unreachable.
- bewaretheirs 5y ago
  The compiler can't know if page 0 is mapped.
- mpweiher 5y ago
  Nope.
- nkurz 5y ago
  Are you familiar with this classic example? http://blog.llvm.org/2011/05/what-every-c-programmer-should-know_14.html http://blog.llvm.org/2011/05/what-every-c-programmer-should-... The problem (if I understand it right) is that "Redundant Null Check Elimination" might be run first, and will get rid of the safety return. But then the "Dead Code Elimination" can be run, which gets rid of the unused read, and thus removes the crash. Which means that rather than being logically equivalent, you can end up with a situation where /* do real stuff */ (aka /* launch missiles */) can be run despite the programmer's clear intentions.
  - Dylan16807 5y ago
    Right, each optimization is fine on its own but the combination is dangerous.
- saagarjha 5y ago
  Not really. My understanding of the opinion (which I don't share, FWIW, and you probably know this) is that the null dereference should not be deleted, and it should be marked as an undeleteable trap similar to what an out-of-bounds access might be in Rust. That is, not unreachable in the sense of "code can never reach here", but unreachable in the sense that if it is hit then no other code executes.

nkurz 5y ago

> "No, no, no! Do the check first, and dereference only when it's not null!" I don't think I've heard anyone express this preference. If they did, I'd agree with you that they are being unreasonable. My impression is that practically everyone on the "portable assembler" team would think it's perfectly reasonable to attempt the read, take the SIGSEGV if x is NULL, and crash if it's not handled. Most would also be happy skipping the read if the value can be shown to be unused. None would be happy with skipping the conditional return. Where it gets thornier is when there is "time travel" involved. What if the unprotected read occurs later, and instead of just returning on NULL, we want to log the error and fall through: void foo(int *x) { if (!x) /* log error */; int val = *x; return; } Is it reasonable to have the later unconditional read of x cause the compiler to skip the earlier check of whether x is NULL? In the absence of an exit() or return in the error handler, I think it would be legal for the compiler to skip both the error logging and the unused load and silently return, but I don't want the compiler to do this. I want it to log the error I asked it to (and then optionally crash) so I can identify the problem. Alternatively, I'd probably be happy with some compile time warning that tells me what I did wrong.

nlewycky 5y ago
How do you feel about: void foo1(int *p) { *p = 7; printf("%d\n", *p); free(p); } May the compiler replace that with puts("7") and free()? Recall that free() is a no-op when the pointer is NULL. Are you arguing that each function may reduce pointer accesses down to one but not down to zero? What about int foo2() { int *p = malloc(400); *p = 0; /* ... code here was inlined then optimized away ... */ ret = 7; free(p); return ret; } ? May we remove '*p = 0;', whether we remove the malloc+free or not? Generally speaking the compiler tries not to reason about the whole function but just to look at the smallest possible collection of instructions, like how add(x, x) can be pattern matched to mul(x, 2) and so on. Having to reduce to one but not zero memory accesses per function is not easily compatible with that model. We would have to model branches that make the access conditional, the length of accesses may differ (what if 'p' is cast to char* and loaded), read vs. write, multiple accesses with different alignment, and maybe other things I haven't considered. Both gcc and clang provide -fsanitize=null which checks all pointer accesses for null-ness before performing them. These can be surprising, your code (libraries you didn't write, headers you included) may be dereferencing NULL and relying on the optimizer to remove the invalid access. IMO there should be a "pointer is null-checked after use", it's a clear example where the programmer wrote something other than what they intended.

jwatte 5y ago

I would expect the code to crash when passed a null pointer, absolutely! And the if statement is there to let me recover after using "continue" in the debugger. And if I run on an ISA without protected memory, that load will not actually crash, and I'm OK with that. That's a level of UB differing-behavior (more like ID, really) that's reasonable. I know of no experienced C programmer who would expect the compiler to re-order the statements. That sounds very much like a strawman argument to me! I'd also be OK with a compiler that emitted a warning: "This comparison looks dumb because the rules say it should do nothing." Emitting that warning is helpful to me, the programmer, to understand where I may have made a mistake. Silently hiding the mistake and cutting out parts of code I wrote, is not generally helpful, even if there exist some benchmark where some inner loop will gain 0.01% of performance if you do so. After all: The end goal is to produce and operate programs that run reliably and robustly with good performance, with minimum programmer cost. Saying that the possible performance improvements of code that nobody will run because it's buggy absolutely trump every other concern in software development, would be a statement I don't think reflects the needs of the software development profession.