It seems that edk2.groups.io <http://edk2.groups.io/> has hit the limits for message size due to quotation levels (?), so I send another copy of my previous message to have it visible from the web-interface.
Andrew, It is ok, as we are all here for mutual benefit, no worries. But you are right that we indeed want a solution for BZ 2054[1] to land as early as possible. It is great that after more than half a year :) we have some productive discussion, so I am all open for it. * The compliant about a breaking change in DebugLib is honestly fair, and I wonder whether we could avoid it. The reason it potentially breaks is the introduction of DebugConstraintAssertEnabled function that will need to land in every DebugLib implementation. I think we can look differently at this problem. Currently functions like DebugAssertEnabled, DebugPrintEnabled, DebugClearMemoryEnabled, and so on are copy-pasted from one DebugLib to another introducing almost 100 lines of exactly the same code every DebugLib implementation should write. It can be argued that one can implement these functions differently, but neither the description in DebugLib.h permits this, nor I have seen any implementation doing it so far. I believe we should check Pcd values directly in DebugLib.h header, which has a lot of pros including the backwards compatibility resolution: — reduction of code copy-pasting. — making compilers without LTO produce smaller binaries. — making third-party DebugLib implementations more flexible and easier to maintain. — resolving the problem of third-party DebugLib implementations not working with CONSTRAINT_ASSERT. To make it less intrusive we can provide an iterative approach: 1. Introduce Pcd checking macros like DEBUG_PRINT_ENABLED, DEBUG_ASSERT_ENABLED: #define DEBUG_PRINT_ENABLED() (BOOLEAN) ((PcdGet8(PcdDebugPropertyMask) & DEBUG_PROPERTY_DEBUG_PRINT_ENABLED) != 0) 2. Switch DEBUG, ASSERT, and other related macros to use these new macros by replacing calls like DebugPrintEnabled() with macros like DEBUG_PRINT_ENABLED(). 3. Introduce DEBUG_CONSTRAINT_ASSERT_ENABLED and CONSTRAINT_ASSERT as discussed previously except DebugConstraintAssertEnabled is now a macro. 4. Remove DebugPrintEnabled and other functions from EDK II DebugLib implementations and header. Step 4 can be done through deprecation phase. However, I think these functions are not used anywhere but inside DebugLib implementations anyway, and the way they are structured should let still legacy third-party DebugLib implementations still having these functions to compile fine even without the immediate refactoring. * The point of AssertMacros and changing the world is valid, but to be frank with you I have an opinion that we should not incorporate this experience in EDK II. I do not know if these macros are used in Mac EFI code, as my only experience with them was during XNU kernelspace programming, but I do not have a memory of them bringing enough benefit. Basically, macro constructions that cannot be easily expressed with normal C functions, especially ones changing control flow with something like a goto, are unintuitive and overcomplicated. They make the code harder to read, especially to those not familiar with the rest of the codebase, for little to no reason. While there certainly may be some positive sides in these macros, like improved structural separation by introducing different categories of checks, I am afraid we do not need them as is. Even if we do implement something close in the future, I believe they should rather exist in a separate library and be an opt-in for newly contributed code. * The point of caller/callee control on the constraint violation is slightly tricky. We have already spent some time trying to nail it down, and I believe there is a good level of logic that can be split in two parts: 1. Assertion type choose. Which to write ASSERT or CONSTRAINT_ASSERT? You can rely on a simple thought experiment here. Does failing this assertion make the function work in the conditions it was not meant to be? I.e. will the function break or crash or will some other spec/doc-compliant implementations of this potentially do. When the answer is affirmative, it is an ASSERT, otherwise it is a CONSTRAINT_ASSERT. Examples in the BZ mentioning SafeString give a good grasp of the use cases. 2. Assertion handling choice. When do we want ASSERT or CONSTRAINT_ASSERT to be enabled? This is more likely what your question was about. First is that CONSTRAINT_ASSERTs only make sense to enable in DEBUG builds where ASSERTs are enabled. So the question comes to whether DEBUG builds should behave the same way as RELEASE builds functionality-wise or not. Let’s take an another simple thought experiment: should we halt in a build when external untrusted data theoretically exists and is not valid? If the answer is yes, then CONSTRAINT_ASSERT should be enabled in a DEBUG build, otherwise no. Basically you enable CONSTRAINT_ASSERTs when you e.g. debug your code trying to find a problem (let’s say a typo) and disable CONSTRAINT_ASSERTs when you give a DEBUG build to the end user for a more or less long-term usage. Sorry for the long read, but I hope I answered all the questions in consideration. Best wishes, Vitaly -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#54499): https://edk2.groups.io/g/devel/message/54499 Mute This Topic: https://groups.io/mt/69401948/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
signature.asc
Description: OpenPGP digital signature
