GCC: remove -fno-delete-null-pointer-checks

[kjbracey]

Summary of changes

For GCC we're being cautious by passing the -fno-delete-null-pointer-checks. This option prevents some optimisation opportunities, so removing it can reduce code size.

One particular optimisation loss occurs in Callback where a test similar to this occurs:

extern void myfunc();

inline void foo(void (*fnptr)())
{
    if (fnptr) {
        do A;
    } else {
        do B;
    }
};

foo(myfunc);

With -fno-delete-null-pointer-checks, the compiler does not assume that &myfunc is non-null, and inserts the "null check" - seeing if the address is 0. But performing that test of the address is incorrect anyway - if myfunc actually could be at address 0, we'd still want to doA.

Anyway, we do not have an equivalent option enabled for either Clang or IAR, and we have performed clean-ups avoiding issues with apparently-null vector tables in Clang already, for example #10534.

Therefore it should(TM) be safe to remove the option for GCC. We do not have general data or code at address 0, only vectors are likely to be there, so it does not make sense to be globally restricting code generation for that.

Impact of changes

Will reduce image size. Particularly when Callback is in use. Increased optimisation may require code adjustments, exposing undefined behaviour

Migration actions required

If code fails, address undefined behaviour. Eg this null check may now be removed:

  int foo(int *ptr)
  {
       int a = ptr[0];
       if (!ptr) {  // may be optimised out - compiler can assume non-null because already accessed
            return -1;
       }
       return bar(a);
  }

Correct to:

  int foo(int *ptr)
  {
       if (!ptr) {
            return -1;
       }
       int a = ptr[0];
       return bar(a);
  }

Documentation

None

---

Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[X] Major update (Breaking change E.g. Return code change / API behaviour change)

---

Test results

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

---

Reviewers

---

[kjbracey]

Something to consider - I'll need to check what the saving looks like. It will be fairly small; we'll have to balance chance of compiler doing something unexpected versus that saving.

[ciarmcom]

@kjbracey-arm, thank you for your changes.
@ARMmbed/mbed-os-tools @ARMmbed/mbed-os-maintainers please review.

[bulislaw]

Should we leave it there in debug?

[kjbracey]

Should we leave it there in debug?

Hard to say. I can see downsides with either choice. My inclination is to just let GCC default to whatever judgement it makes for -O0 or -O1. (-O is a master control switch for the default state of dozens of optimisations like that.)

I've checked, and saving on a full client build is:

| Subtotals                                     | 411956(-320) | 3360(+0) | 54376(+0) |

That's with current Callback - it's possible it might synergise with my other Callback work.

[adbridge]

@bulislaw @kjbracey-arm what is the status of this now ?

[kjbracey]

I think it needs discussion/review. Might be worth doing 1 CI run to see if any issues are seen - I'd hope it's green straight off.

[0xc0170]

CI started

[mbed-ci]

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

• jenkins-ci/mbed-os-ci_greentea-test

[kjbracey]

Could you retrigger CI? I think that was a spurious failure. Review/discussion still required.

[bulislaw]

Approved after talking to @kjbracey-arm.

[adbridge]

CI started

[mbed-ci]

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 2
Build artifacts

[mergify]

This PR does not contain release version label after merging.