Reworked disabling error windows by introducing new defines so that we can more eas…#2352
Conversation
…ily enable the code for Embarcadero C++ Builder
* ACE/ace/Object_Manager.cpp:
* ACE/ace/config-win32-borland.h:
* ACE/ace/config-win32-mingw.h:
* ACE/ace/config-win32-mingw64.h:
* ACE/ace/config-win32-msvc.h:
WalkthroughThis pull request modifies the conditional compilation in the Windows-specific error reporting mechanism of the ACE_Object_Manager. It streamlines the conditions by relying solely on Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner as Test Runner
participant SEHBlock as SEH Block
participant Logger as Logger
TestRunner->>Logger: Check for SEH support via ACE_HAS_WIN32_STRUCTURED_EXCEPTIONS
alt SEH Supported
TestRunner->>SEHBlock: Enter ACE_SEH_TRY block
SEHBlock->>TestRunner: Trigger access violation (null pointer)
TestRunner->>SEHBlock: Transition to ACE_SEH_FINALLY block
SEHBlock->>Logger: Log execution of finally block
TestRunner->>Logger: Report test passed
else SEH Not Supported
TestRunner->>Logger: Log non-support of SEH
end
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
🔇 Additional comments (8)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ACE/ace/Object_Manager.cpp (1)
292-317: Enhanced conditional compilation for exception filter handlingThe code now uses the new
ACE_HAS_SETUNHANDLEDEXCEPTIONFILTERdefine along with architecture checking to properly isolate Intel x86-specific code. This makes the implementation more robust and maintainable.One suggestion to improve readability:
The comment on line 292 indicating "this only works for intel based windows builds" could be made clearer by mentioning that it's specifically for 32-bit Intel (x86) architectures, not 64-bit (x64/AMD64). Consider:
- // NOTE this only works for intel based windows builds. + // NOTE: This only works for 32-bit Intel (x86) Windows builds, not 64-bit (x64/AMD64).
🛑 Comments failed to post (1)
ACE/tests/Compiler_Features_41_Test.cpp (1)
1-61: 🛠️ Refactor suggestion
Well-structured test for SEH functionality
This test effectively verifies structured exception handling (SEH) support, which is relevant to the PR's modifications around exception handling. The implementation follows good practices:
- The test is properly guarded with
ACE_HAS_WIN32_STRUCTURED_EXCEPTIONSto ensure it only runs on supported platforms- It uses deliberate null pointer dereference to test SEH handling
- It verifies that the
FINALLYblock executes as expectedHowever, there is a potential issue with the test's recovery mechanism:
The test intentionally causes an access violation but doesn't include an explicit try/catch or SEH
EXCEPTblock to handle the exception. While theFINALLYblock will execute, the test might still crash on some platforms depending on how SEH is implemented.Consider either:
- Adding explicit exception handling with
ACE_SEH_EXCEPTto catch the exception, or- Adding a comment explaining why this approach is safe across all targeted platforms
ACE_SEH_TRY { ACE_DEBUG ((LM_DEBUG,("In SEH_TRY\n"))); // Intentionally cause an access violation exception char *cause_exception {}; *cause_exception = 'x'; // Should raise an exception // If we get here without an exception, SEH isn't working ACE_ERROR ((LM_ERROR, "No exception was raised from null pointer dereference\n")); result = -1; } +ACE_SEH_EXCEPT(EXCEPTION_EXECUTE_HANDLER) + { + // Exception was caught as expected + ACE_DEBUG ((LM_DEBUG, "Exception caught as expected\n")); + } ACE_SEH_FINALLY { ACE_DEBUG ((LM_DEBUG,("In SEH_FINALLY\n"))); finally_executed = true; // If we're here due to an exception, that's the expected behavior - test passes }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./** * This program checks if the compiler/RTL does have correct support * for structured exception handling using __try/__finally */ #include "test_config.h" int run_main (int, ACE_TCHAR *[]) { ACE_START_TEST (ACE_TEXT("Compiler_Features_41_Test")); int result = 0; #if defined (ACE_HAS_WIN32_STRUCTURED_EXCEPTIONS) bool finally_executed = false; ACE_DEBUG ((LM_DEBUG,("Testing __try/__finally\n"))); ACE_SEH_TRY { ACE_DEBUG ((LM_DEBUG,("In SEH_TRY\n"))); // Intentionally cause an access violation exception char *cause_exception {}; *cause_exception = 'x'; // Should raise an exception // If we get here without an exception, SEH isn't working ACE_ERROR ((LM_ERROR, "No exception was raised from null pointer dereference\n")); result = -1; } ACE_SEH_EXCEPT(EXCEPTION_EXECUTE_HANDLER) { // Exception was caught as expected ACE_DEBUG ((LM_DEBUG, "Exception caught as expected\n")); } ACE_SEH_FINALLY { ACE_DEBUG ((LM_DEBUG,("In SEH_FINALLY\n"))); finally_executed = true; // If we're here due to an exception, that's the expected behavior - test passes } ACE_DEBUG ((LM_DEBUG,("After SEH_FINALLY\n"))); // On successful SEH handling: // 1. The null pointer dereference should raise an exception // 2. The FINALLY block should execute // 3. Execution should continue after the SEH blocks if (finally_executed) { ACE_DEBUG ((LM_DEBUG, "SEH test passed - FINALLY block was executed\n")); } else { ACE_ERROR ((LM_ERROR, "SEH test failed - FINALLY block was not executed\n")); result = -1; } #else ACE_DEBUG ((LM_INFO, ACE_TEXT ("Platform lacks ACE_HAS_WIN32_STRUCTURED_EXCEPTIONS\n"))); #endif /* ACE_HAS_WIN32_STRUCTURED_EXCEPTIONS */ ACE_END_TEST; return result; }
…ily enable the code for Embarcadero C++ Builder
Summary by CodeRabbit