4 ms·

> "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

by 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.
- hramrach 5y ago
  Don't compilers already have ways to mark variables and dereferences in a way to say 'I really want access to this value happen'? They are free to optimize away access to any result that is not used based on code elimination, and these annotations limit what can be eliminated. However, basing code elimination on UB, as pointed out earlier, would basically eliminate all code if you were rigorous enough because you basically cannot eliminate UB in C code, which is not in any way useful.
  - Joky 5y ago
    > Don't compilers already have ways to mark variables and dereferences in a way to say 'I really want access to this value happen'? The standard defines it, it's `volatile` I believe. But it does not help with the examples above as far as I understand (removing the log, removing the early return, time-travel...).
    - hramrach 5y ago
      I believe it completely solves the question ? May we remove '*p = 0;', whether we remove the malloc+free or not? Sure, it does not solve the question when arbitrarily removing NULL pointer checks is OK. It is true that when the compiler is inlining code or expanding a macro it may have a NULL check that is spurious in environments that do not map page 0 based on the observation that the pointer was dereferenced previously. And this assumption is incorrect in environments that do map page 0 causing wrong code generation.
- nkurz 5y ago
  Quick responses before I go to bed: I think compilers should emit code that make writes to memory when the program asks them to, even if the pointer is then freed. Not doing so too easily leads to security issues. If the pointer turns out to be NULL at runtime, then let it crash. Using 'volatile' often works as a workaround, though. I have no strong opinion on substituting puts() for printf(). I think incorporating the standard library into the spec was unhelpful, but isn't a major issue either way. It occasionally makes tracing slightly more difficult, but presumably helps with performance enough to offset this, and it's never bitten me as badly as the UB optimizations have.

by nkurz 5y ago

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.

hramrach 5y ago
Don't compilers already have ways to mark variables and dereferences in a way to say 'I really want access to this value happen'? They are free to optimize away access to any result that is not used based on code elimination, and these annotations limit what can be eliminated. However, basing code elimination on UB, as pointed out earlier, would basically eliminate all code if you were rigorous enough because you basically cannot eliminate UB in C code, which is not in any way useful.
- Joky 5y ago
  > Don't compilers already have ways to mark variables and dereferences in a way to say 'I really want access to this value happen'? The standard defines it, it's `volatile` I believe. But it does not help with the examples above as far as I understand (removing the log, removing the early return, time-travel...).
  - hramrach 5y ago
    I believe it completely solves the question ? May we remove '*p = 0;', whether we remove the malloc+free or not? Sure, it does not solve the question when arbitrarily removing NULL pointer checks is OK. It is true that when the compiler is inlining code or expanding a macro it may have a NULL check that is spurious in environments that do not map page 0 based on the observation that the pointer was dereferenced previously. And this assumption is incorrect in environments that do map page 0 causing wrong code generation.
nkurz 5y ago
Quick responses before I go to bed: I think compilers should emit code that make writes to memory when the program asks them to, even if the pointer is then freed. Not doing so too easily leads to security issues. If the pointer turns out to be NULL at runtime, then let it crash. Using 'volatile' often works as a workaround, though. I have no strong opinion on substituting puts() for printf(). I think incorporating the standard library into the spec was unhelpful, but isn't a major issue either way. It occasionally makes tracing slightly more difficult, but presumably helps with performance enough to offset this, and it's never bitten me as badly as the UB optimizations have.