Thanks Sandrine, Louis,
Agree with both of you. I'm fine with using asserts or panics, as long as we uniformly use it. The change i sent out(review 3845) was because i noticed inconsistency in handling errors. If we do use asserts, all code that checks for mandatory properties, should be changed to assert as opposed to return error code. For optional properties, we can continue to return an error code or print warnings etc.
I would like to point out that using asserts here is different from what is documented in the coding guidelines. The guidelines explicitly spells out using asserts for "programming errors". Now, is having a bad DTB considered a programming error? ;) The DTB is platform data as opposed to code. The guidelines might need to be updated based on the conclusion here. Also note the couple of scenarios i mentioned in an earlier email. Platforms like RPI4 don't generally enable TBBR and the DTB image could get corrupt or be modified on purpose. On a release build, this could cause silent corruption. Panic() would avoid this.
In any case, it would be good to settle on the expected behavior for each of these abnormal cases. I don't have a strong preference for asserts or panics here, since each has its pros and cons as both of you called out.
Thanks Raghu
On 4/7/20 3:34 AM, Sandrine Bailleux wrote:
Hi Raghu and Louis,
On 4/7/20 12:14 PM, Louis Mayencourt via TF-A wrote:
I do agree with you: case 2 and 3 are similar (wrongly formed DTB) and should lead to the same behavior.
A mandatory property miss or a hit with a structurally incorrect node means that the DTB doesn't follow the provided binding document. Such a DTB shouldn't be considered as valid and should trigger a build failure and/or a code panic.
That's what still confuses me... Agree on cases 2 and 3 triggering a build failure if possible, but not a code panic. A code panic stays in a release build. With what we've been discussing so far, it would seem more appropriate to me to have debug assertions to catch cases 2 and 3. These debug assertions can help catching structural problems in the DTB during the development phase and can be eliminated for a production build, leaving no checks whatsoever in the code.
This is the strategy we've been using so far in TF-A. For lots of platform interfaces, the generic code includes debug assertions to check the correct implementation of these interfaces by platform integrators. For example, checking the range of their return values. I would say this is deeply embedded into the threat model TF-A uses today. See https://trustedfirmware-a.readthedocs.io/en/latest/process/coding-guidelines...
On one hand, it makes sense to me. On the other hand, I take Raghu's point that it would be unrealistic to assume that 100% of code has been covered by tests. This is very hard to achieve in practice, especially to cover all error cases ; thus, it seems utopian to assume that all debug assertions have been exercised during development and can be safely removed.
TF-A does provide a way to keep debug assertions in a release build (using the ENABLE_ASSERTIONS build flag) if platform integrators judge they would rather keep them but this is not the default behaviour.
Regards, Sandrine