Skip to content

Conversation

JanneKiiskila
Copy link
Contributor

Description

Decrease optimisation level from 3 to 2. Reason being that we are
seeing multiple compiler issues with level 3 and this is the workaround
to get around those. We assume the impact on any performance/size will
be non-visible to anyone.

This is a workaround for IOTCLT-2038 / SingletonPtr corrupts.

Status

READY

Migrations

If this PR changes any APIs or behaviors, give a short description of what API users should do when this PR is merged.

NO

Related PRs

IOTCLT-2038

Decrease optimisation level from 3 to 2. Reason being that we are
seeing multiple compiler issues with level 3 and this is the workaround
to get around those. We assume the impact on any performance/size will
be non-visible to anyone.

This is a workaround for IOTCLT-2038 / SingletonPtr corrupts.
@adbridge
Copy link
Contributor

adbridge commented Oct 3, 2017

@sg- @theotherjimmy can you please review this? Do we have any issue with reducing the optimisation level ?

@sg-
Copy link
Contributor

sg- commented Oct 3, 2017

No way. Wait for a compiler fix or update the software such that the optimizations dont occur.

Copy link
Contributor

@theotherjimmy theotherjimmy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make @sg- comment a red x. It was meant that way.

@pan-
Copy link
Member

pan- commented Oct 3, 2017

Looking at the issue, what are the proofs that it is an ARMCC issue ?

@JanneKiiskila
Copy link
Contributor Author

Same SW works with GCC and IAR. Same SW works with ARM CC without optimisations and with -O2, but not with -O3.

@JanneKiiskila
Copy link
Contributor Author

JanneKiiskila commented Oct 3, 2017

So, what are we gaining with -O3 compared to -O2? Comment "No way" doesn't bear any arguments why the -O3 is such a significant requirement.

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 3, 2017

So, what are we gaining with -O3 compared to -O2? Comment "No way" doesn't bear any arguments why the -O3 is such a significant requirement.

This has been here for years. It has huge impact. This type of change must have strong arguments.

From the experience, there are sometimes compiler issues, but very rarely. I remember once we had one file that was causing compiler error with O3, the file ended up with a workaround (lowering optimiziation just in one file - that was even for specific compiler versions that had that bug in - thus very specific. I believe what you are experiencing is different, having runtime failures somewhere in the software stack. This could indicate that something might have undefinied/implementation specific problem?

What issues are with ARMCC -O3 ?

@pan-
Copy link
Member

pan- commented Oct 4, 2017

@JanneKiiskila I'm not saying it's not an ARMCC issue however the more optimizations are in, the more latent undefined behavior gets exhibited and usually not in a trivial way. So I was wondering if you have a detailed explanation for the bug you experience and not just a workaround.

@JanneKiiskila
Copy link
Contributor Author

JanneKiiskila commented Oct 4, 2017

We're working on it, I hope we can nail it down in the very near future.

@pan-
Copy link
Member

pan- commented Oct 4, 2017

From my understanding, the assertion can be false only if SingletonPtr::_ptr is not correctly zero initialized or corrupted (via a buffer overflow). For both case, it may be interesting to put a watchpoint which monitor write on the memory address containing _ptr.

@JanneKiiskila
Copy link
Contributor Author

Yes, that's on-going right now - seems to be just a tad bit difficult to get the stack backtrace intact.

@JanneKiiskila
Copy link
Contributor Author

Root cause is stack overflow - the SHA256 routines from MbedTLS allocate an array of 64 uint32s and well, these overflow. This happens only with ARM CC -O3 optimisation, that seems to use more memory than any other compiler/option - but then again our compiler settings aren't similar in Mbed OS.

We will fix this by increasing main thread size by 512 bytes (in all configs, as we will not make compiler specific mbed_app.json -files, too large overhead).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants