Updated support for Green Hills INTEGRITY and INTEGRITY-178 RTOS #2340
Conversation
Includes INTEGRITY RTOS and INTEGRITY-178 tuMP
- Make INTEGRITY's config file conditional on whether POSIX calls are available
WalkthroughThe pull request introduces extensive updates across the ACE and TAO codebases. Changes include new ignore patterns in the repository configuration; enhancements to signal, thread, and memory operations; and improved handling of string views, file flags, and network IP conversion. Numerous preprocessor and configuration modifications standardize macro usage—primarily replacing legacy INTEGRITY macros with ACE_INTEGRITY—and a new single input reactor feature is added and integrated into the TAO resource factory. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TAO_Factory
participant SingleReactor
Client->>TAO_Factory: Request reactor with "single_input" flag
TAO_Factory->>SingleReactor: Instantiate ACE_Single_Input_Reactor
SingleReactor->>Client: Set up event handling
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.31.1)ACE/ace/OS_NS_Thread.cpp✨ Finishing Touches
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (28)
ACE/ace/OS_NS_arpa_inet.cpp (1)
102-118: Added INTEGRITY-specific implementation of inet_ntoaThe implementation correctly converts an IPv4 address from network representation to a string. This implementation is needed to support systems where the standard function is missing.
Consider removing the commented-out debug printf statement on line 109 as it's not providing value in production code.
- // printf("Socket address %X, IP address %X.\n",addr.s_addr,ipaddr);Also, be aware that this implementation uses a static buffer which could cause issues in multi-threaded environments. If INTEGRITY uses this in multi-threaded contexts, consider using thread-local storage or requiring callers to provide their own buffer.
ACE/ace/OS_NS_sys_socket.inl (1)
949-949: Minor cleanup of preprocessor directive.The removal of a comment from the
#endifdirective makes the code cleaner and more consistent with other preprocessor directives in the file.ACE/ace/OS_NS_arpa_inet.inl (1)
12-45: Check range bounds in custom inet_addr implementation.The loop correctly parses up to four IP segments, but does not verify that each segment ≤ 255. Larger values (e.g., 999) will be silently included in the final address, leading to incorrect IP resolution. Consider adding a validation step:
while (*name >= '0' && *name <= '9') { segment *= 10; segment += *name++ - '0'; + if (segment > 255) { + valid = 0; + break; + } }ACE/ace/config-integrity.h (1)
62-62: Consider a more robust fileno implementation.The fileno macro implementation is a hack (as noted in the comment) to get Svc_Conf_l.cpp compiled. While this works, consider a more robust solution for file descriptor handling in the future to reduce technical debt.
ACE/ace/Flag_Manip.cpp (3)
24-57: Encapsulated flag manipulation logic looks clean.
The introduction ofset_flags_helper()andclr_flags_helper()in an unnamed namespace is a neat way to centralize and streamline flag updates. It improves maintainability by eliminating repeatedfcntlcalls throughout the code. However, consider adding minimal debug logging or error handling to provide actionable diagnostics for failure cases (e.g., whenfcntlreturns-1), especially in production builds.
80-106: Nested preprocessor conditions may reduce readability.
Although this block correctly handles multiple edge cases (e.g.,ACE_LACKS_IOCTLvs. fallback toioctl), the nested#ifandswitchstatements can become hard to follow as the codebase grows. Consider refactoring or documenting the logic more clearly, possibly by extracting platform-specific code into separate functions.
130-157: Ensure robust error handling for I/O calls.
Like the set-flags case, theclr_flagslogic is heavily guarded by#ifdirectives. The approach is valid, but ifposix_devctlorioctlfail, the code returns the result without further commentary. You might consider logging or capturingerrnoto facilitate debugging in production or test environments.ACE/ace/config-integrity178.h (1)
117-123: Thread-specific storage emulation.
DefiningACE_HAS_TSS_EMULATIONfor INTEGRITY-178B is appropriate given the lack of direct TSS APIs. Keep in mind that TSS emulation may introduce additional overhead. If performance becomes a concern, investigate alternative concurrency patterns or OS-level hooks if Green Hills provides them for the 178B environment.ACE/ace/config-integritySCA.h (2)
25-34: Commented-out macros for user/group operations.
Leaving these macros commented out (e.g.,ACE_LACKS_SETEGID,ACE_LACKS_SETEUID) might cause confusion unless documented. Clarify whether developers should uncomment them if an older integrity environment truly lacks these features, or remove them entirely to avoid stale code.
92-101: Wide character function omissions.
Macros likeACE_LACKS_WCSICMP,ACE_LACKS_WCSNICMP, andACE_LACKS_WCSDUPare common in embedded or specialized RTOS setups. If applications require wide-char functionality, consider providing stubs or alternative implementations to gracefully handle references to these functions.ACE/ace/OS_NS_Thread.h (2)
43-48: Remove unused<map>include.
It appears<map>is no longer needed since you've opted for a static array implementation for TSS. Removing it can reduce clutter and potential confusion.# if defined (INTEGRITY) # include "ace/Log_Msg.h" -# include <map> # endif
290-367: Minor comment grammatical fix & future expansion.
- Update the comment “already insert this entry” to “already inserted this entry.”
- If you need to support more than
ACE_DEFAULT_THREADStasks or a dynamic approach, consider using ACE’s map or a custom structure and remove the TODO in the comments.Proposed comment fix:
- // Earlier call has already insert this entry. + // An earlier call has already inserted this entry.ACE/ace/OS_NS_Thread.cpp (2)
1567-1573: INTEGRITY-specific branch for cond_timedwait.
Currently returnsACE_NOTSUP_RETURN(-1), indicating no implementation. If you plan to support timed waits later, consider providing at least partial functionality.If you need help implementing a partial emulation using available INTEGRITY calls, let us know.
3300-3442: New INTEGRITY namespace block for TSS workaround.
This sizable insertion for INTEGRITY/INTEGRITY-178B manages TSS via a global map and custom adapters. The strategy looks reasonable, but be sure to stress-test concurrency scenarios and memory usage.Consider adding unit tests specifically covering corner cases such as repeated TSS sets and cross-thread resource cleanup.
ACE/ace/OS_NS_Thread.inl (8)
1445-1450: Consolidate 'not supported' code.Under INTEGRITY, returning
ACE_NOTSUP_RETURN(-1)forsema_destroy()is reasonable for now. If full semaphore cleanup becomes a requirement, consider implementing or simulating the missing operation rather than leaving it permanently unsupported.
2067-2082: Implement wait with concurrency safety.Here, the code for INTEGRITY178B manually checks for underflow and returns
-1if the semaphore is at its limit. Please verify that race conditions where multiple tasks attempt to wait simultaneously are accounted for, and consider whether further synchronization or error handling is needed.
2281-2286: Timeout-based semaphore wait not supported.Returning
ACE_NOTSUP_RETURN(-1)indicates no timeout support for INTEGRITY insema_wait(). If the design requires timed waits in the future, plan to integrate the appropriate OS-level calls or a custom workaround.
2401-2403: Check for partial SIG support.This code for
sigwait()on INTEGRITY is stubbed out withACE_NOTSUP_RETURN(-1). If signals are necessary, consider emulating the behavior or gracefully ignoring them to avoid unexpected runtime issues.
2755-2757: Confirm thr_kill equivalence.Using
::ExitTask(thr_id, (Value)signum)to emulatethr_killis effectively forcing thread termination. Confirm that forced exit fits your cleanup model and doesn’t bypass destructors or resource deallocation, especially if signals were originally used for graceful interruption.
3210-3211: Assess yield correctness.Replacing
thr_yield()with::Yield()can change scheduling behavior. Confirm that this implementation meets your fairness or real-time requirements, as some OS-level yields may be no-ops or exhibit unexpected side effects.
3266-3270: Thread mutex init stub.The code ignores attributes such as
lock_type,name, or extended parameters. If external components rely on specialized mutex attributes, consider implementing them for more fine-grained synchronization control.
3312-3316: Add extended mutex attribute support if needed.Similar to the previous stub, this code discards the lock attributes. If advanced concurrency or debugging features become essential, revisit this area for a more comprehensive initialization approach.
ACE/ace/Single_Input_Reactor.cpp (4)
12-16: Prefer explicit boolean.
initialized()returns1, although the return type is effectively boolean. Consider returningtrueto avoid confusion and to make the intent clearer.
17-20: Clarify meaning of size.
size()returns1if an event handler is set, otherwise0. For readability, consider returning a strictly unsigned integer (0 or 1), or rename the method to reflect that it indicates presence or absence of a handler. If future expansions allow more handlers, refactor accordingly.
37-44: Check error indications from handle_input.When invoking
handler->handle_input(), the return code can signal closure or errors. Evaluate whether you need to handle such indications (e.g., removing the handler if it requests shutdown).
46-50: Assess concurrency with Null_Mutex.Returning a lock adapter around
ACE_Null_Mutexmeans no real synchronization. Confirm that no parallel threads will concurrently call methods that could interfere with each other or disrupt reactor state.ACE/ace/Single_Input_Reactor.h (2)
1-12: Expand documentation on limitations.The top comment clarifies the single-input nature but may not fully describe other constraints like lack of notification or timeout handling. Document these limitations thoroughly so that users don’t accidentally rely on unsupported features.
164-166: Potential future extensions.Currently, only one
ACE_Event_Handler_varis stored. If your use case evolves to handle multiple input sources or more complex logic, this approach must be revisited to avoid significant refactoring later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
.gitignore(2 hunks)ACE/ace/ACE.inl(2 hunks)ACE/ace/Asynch_Pseudo_Task.cpp(1 hunks)ACE/ace/CDR_Size.h(2 hunks)ACE/ace/CDR_Size.inl(3 hunks)ACE/ace/CDR_Stream.cpp(3 hunks)ACE/ace/CDR_Stream.h(3 hunks)ACE/ace/CDR_Stream.inl(3 hunks)ACE/ace/Flag_Manip.cpp(2 hunks)ACE/ace/Malloc_Base.h(1 hunks)ACE/ace/OS_NS_Thread.cpp(19 hunks)ACE/ace/OS_NS_Thread.h(3 hunks)ACE/ace/OS_NS_Thread.inl(23 hunks)ACE/ace/OS_NS_arpa_inet.cpp(1 hunks)ACE/ace/OS_NS_arpa_inet.inl(2 hunks)ACE/ace/OS_NS_dirent.inl(3 hunks)ACE/ace/OS_NS_fcntl.cpp(1 hunks)ACE/ace/OS_NS_signal.inl(6 hunks)ACE/ace/OS_NS_stdlib.cpp(2 hunks)ACE/ace/OS_NS_sys_select.inl(1 hunks)ACE/ace/OS_NS_sys_socket.inl(3 hunks)ACE/ace/OS_NS_sys_stat.inl(4 hunks)ACE/ace/OS_NS_sys_utsname.cpp(1 hunks)ACE/ace/OS_NS_time.h(1 hunks)ACE/ace/OS_NS_unistd.inl(11 hunks)ACE/ace/Object_Manager.cpp(1 hunks)ACE/ace/Object_Manager.h(2 hunks)ACE/ace/Service_Config.cpp(1 hunks)ACE/ace/Single_Input_Reactor.cpp(1 hunks)ACE/ace/Single_Input_Reactor.h(1 hunks)ACE/ace/Task.cpp(1 hunks)ACE/ace/ace.mpc(1 hunks)ACE/ace/config-integrity-common.h(1 hunks)ACE/ace/config-integrity.h(1 hunks)ACE/ace/config-integrity178.h(1 hunks)ACE/ace/config-integritySCA.h(7 hunks)ACE/ace/config-macros.h(2 hunks)ACE/ace/os_include/os_fcntl.h(1 hunks)ACE/ace/os_include/sys/os_types.h(1 hunks)ACE/tests/Dynamic_Priority_Test.cpp(1 hunks)ACE/tests/Task_Test.cpp(1 hunks)TAO/tao/IIOP_Acceptor.cpp(1 hunks)TAO/tao/Strategies/advanced_resource.cpp(3 hunks)TAO/tao/Strategies/advanced_resource.h(1 hunks)TAO/tao/default_resource.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- TAO/tao/IIOP_Acceptor.cpp
- ACE/ace/Task.cpp
- ACE/ace/OS_NS_sys_select.inl
🧰 Additional context used
🪛 Cppcheck (2.10-2)
ACE/ace/Single_Input_Reactor.cpp
[error] 21-21: There is an unknown macro here somewhere. Configuration is required. If ACE_BEGIN_VERSIONED_NAMESPACE_DECL is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: ubuntu-20.04 g++-10 ACE for TAO
- GitHub Check: ubuntu-20.04 clang++-11
- GitHub Check: ubuntu-20.04 clang++-10
- GitHub Check: VS2022Release64
- GitHub Check: VS2022Debug64
- GitHub Check: VS2022WChar
- GitHub Check: VS2019Debug64Cxx20
- GitHub Check: VS2019Debug64Cxx17
- GitHub Check: VS2019Release32
- GitHub Check: VS2019Debug32
- GitHub Check: VS2019Release64
- GitHub Check: VS2019Debug64
- GitHub Check: macos-14-C++
- GitHub Check: windows-2019 vs2019
- GitHub Check: VS2019WChar
- GitHub Check: MINGW64-gcc
- GitHub Check: alpine-3.18
- GitHub Check: ubuntu-20.04 g++-10
- GitHub Check: macos-13-C++
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (99)
ACE/ace/ace.mpc (1)
220-220: Looks good: New Single_Input_Reactor source added correctlyThe addition of
Single_Input_Reactor.cppto the source files list is properly placed in alphabetical order within the file list, maintaining the project's organization.ACE/tests/Task_Test.cpp (1)
125-125: Fixed thread count is more appropriate for cross-platform testingChanging from
ACE_MAX_THREADSto a fixed value of 3 is a good approach for consistent testing across different platforms, especially when adding support for RTOS environments where thread resources might be more constrained..gitignore (2)
20-24: Appropriate additions for INTEGRITY environment filesThese new file patterns (*.gpj, *.int, *.cmd, etc.) appear to be specific to the Green Hills INTEGRITY development environment and should be ignored in version control.
40-42: Build directories properly added to ignore listAdding these Visual Studio and static build directories ensures that build artifacts don't get accidentally committed to the repository.
ACE/ace/os_include/os_fcntl.h (1)
102-103: Platform-specific nonblock definition for INTEGRITY178BThis addition correctly handles the INTEGRITY178B-specific case for non-blocking operations by defining
ACE_NONBLOCKas 1, which is essential for proper I/O operations on this platform.ACE/ace/ACE.inl (2)
219-223: Added Green Hills INTEGRITY support to socket operations.The change adds the
__ghs__macro check alongside existingACE_WIN32andACE_MQXmacros to useACE_OS::sendinstead ofACE_OS::writefor Green Hills INTEGRITY RTOS. This properly extends the platform-specific I/O abstraction while maintaining backward compatibility.
229-233: Added Green Hills INTEGRITY support to socket operations.Similar to the send operation, this change adds the
__ghs__macro check to useACE_OS::recvinstead ofACE_OS::readfor Green Hills INTEGRITY RTOS. The implementation correctly maintains platform-specific socket behavior.ACE/ace/os_include/sys/os_types.h (1)
66-67: Added INTEGRITY support for large file offset type.The change adds INTEGRITY to the platforms where
ACE_LOFF_Tis defined aslong long. This ensures consistent large file offset handling across different RTOS platforms.ACE/tests/Dynamic_Priority_Test.cpp (1)
50-52: Added INTEGRITY support to test header inclusion.The change extends the conditional inclusion of
ace/OS_NS_string.hto include cases where theINTEGRITYmacro is defined, similar to the existing support for VXWORKS. This appropriately maintains consistent behavior for string operations across different RTOSes.ACE/ace/Malloc_Base.h (1)
189-191:✅ Verification successful
Added conditional exception handling for Green Hills environment.
The change wraps the
std::bad_allocexception in a conditional check forACE_NO_GHS_EXCEPT, allowing memory allocation failures to be handled differently when necessary for Green Hills INTEGRITY RTOS, which may have different exception handling requirements.Run the following to check for similar exception handling patterns:
🏁 Script executed:
#!/bin/bash # Check for similar exception handling patterns in the codebase echo "Looking for similar exception handling patterns..." rg -A 3 "defined \(ACE_NO_GHS_EXCEPT\)" --type cppLength of output: 1425
Conditional Exception Handling for Green Hills Environment Confirmed
The pattern in ACE/ace/Malloc_Base.h is consistent with similar exception handling found in ACE/ace/CDR_Stream.cpp. The check for
!defined(ACE_NO_GHS_EXCEPT)properly guards the use ofstd::bad_allocto accommodate the differences in Green Hills INTEGRITY RTOS exception handling. No further changes are necessary.
- Location:
ACE/ace/Malloc_Base.h(Lines 189-191)- Context: The code uses a conditional compilation directive to enable exception throwing for memory allocation failure only when
ACE_NO_GHS_EXCEPTis not defined.- Consistency: Similar patterns are verified in
ACE/ace/CDR_Stream.cpp.ACE/ace/Asynch_Pseudo_Task.cpp (1)
52-54: Use ACE_OS namespace for signal functions - good change!The change to use
ACE_OS::sigemptysetandACE_OS::sigaddsetinstead of direct system calls improves platform independence. This is consistent with ACE's design philosophy of providing abstraction layers for system calls, and particularly important for supporting INTEGRITY RTOS platforms.TAO/tao/Strategies/advanced_resource.h (1)
68-68: Added new TAO_REACTOR_SINGLE_INPUT enum valueThe addition of this new reactor type is appropriate for extending the reactor selection mechanism. This cleanly extends the enum without disrupting existing values, maintaining backward compatibility.
Based on the PR objectives, this change appears to be part of adding Green Hills INTEGRITY support, likely by enabling a reactor implementation that works well on this platform.
ACE/ace/OS_NS_time.h (1)
130-135: Improved handling of potentialdifftimemacroThis change improves platform compatibility by addressing cases where
difftimemight be defined as a macro, similar to how other time functions are handled in this file. The pattern matches other helper functions in the same file (ace_asctime_r_helper, etc.).The change follows the established pattern in this file where potentially macro-defined time functions are used and then undefined to avoid naming conflicts, enhancing compatibility with the INTEGRITY platform.
ACE/ace/Object_Manager.cpp (1)
242-245: Appropriate INTEGRITY RTOS support added.The added code correctly preallocates the
ACE_INTEGRITY_TSS_Implobject for the INTEGRITY and INTEGRITY-178 RTOS platforms. The conditional compilation guards ensure this code only activates when necessary (INTEGRITY defined, TSS emulation enabled, and no thread-specific storage).ACE/ace/OS_NS_dirent.inl (3)
25-27: Enhanced error handling for closedir on platforms lacking this functionality.This addition properly handles the case where
ACE_LACKS_CLOSEDIRis defined but none of the other platform-specific conditions are met, preventing compiler warnings about unused parameters.
44-46: Improved platform compatibility for opendir.The added code enhances error handling when
ACE_LACKS_OPENDIRis defined by marking the parameter as unused and returning the appropriate error indicator. This provides better support for platforms like INTEGRITY RTOS that may not support standard directory operations.
64-66: Consistent error handling for readdir function.This change follows the same pattern as the other directory functions, adding proper error handling when
ACE_LACKS_READDIRis defined. Marking the parameter as unused prevents compiler warnings, and returningACE_NOTSUP_RETURN(0)correctly indicates the operation is not supported.TAO/tao/default_resource.cpp (2)
794-794: Proper reactor instance management.Setting the global reactor instance ensures that other parts of the system can access this reactor through
ACE_Reactor::instance(). This is important for proper functioning in environments like INTEGRITY RTOS.
801-801: Clean reactor instance shutdown.Setting the reactor instance to null during cleanup is crucial to avoid dangling pointers and ensure proper shutdown sequence. This change prevents potential issues when the application terminates on the INTEGRITY platform.
ACE/ace/Object_Manager.h (2)
43-45: Forward declaration for INTEGRITY TSS implementation.This forward declaration is appropriately guarded by the same preprocessor conditions as the implementation in Object_Manager.cpp, ensuring consistency across the codebase for INTEGRITY RTOS support.
306-308: INTEGRITY TSS enum value added to Object_Manager.Adding the
ACE_INTEGRITY_TSS_IMPLenum value to thePreallocated_Objectenumeration, with consistent preprocessor guards, correctly completes the integration of INTEGRITY TSS support in the object manager's preallocation system.ACE/ace/CDR_Size.h (2)
82-84: Excellent addition of std::string_view support with proper conditional compilation.The new
write_string_viewmethod is correctly guarded by theACE_LACKS_STD_STRING_VIEWpreprocessor directive, allowing the codebase to supportstd::string_viewwhen available while maintaining backward compatibility with platforms that don't support it yet.
237-240: Well-implemented << operator overload for std::string_view.This operator overload is consistently guarded with the same preprocessor directive as the method implementation, ensuring proper behavior across all platforms.
ACE/ace/OS_NS_stdlib.cpp (3)
1199-1206: Good platform-specific adaptation for INTEGRITY.The implementation correctly handles the
__prognamevariable differently for INTEGRITY platforms, making it a mutable pointer withconst_cast<char *>rather than aconst char*. The conditional compilation with nested checks forINTEGRITYandACE_USES_GHS_ISIMPPCis appropriate.
1223-1227: Consistent INTEGRITY handling in setprogname_emulation.The modification to use
const_castfor INTEGRITY is consistent with the variable declaration change. This allows the INTEGRITY implementation to modify the program name when needed.
1229-1233: Consistent implementation for the else branch.The modifications consistently apply the same pattern to both branches of the conditional, ensuring uniform behavior across different code paths.
ACE/ace/OS_NS_sys_socket.inl (1)
138-141: Good error handling for systems lacking socket close functionality.This change adds proper error handling for platforms that lack the
closefunction for sockets via theACE_LACKS_CLOSEdirective. The implementation correctly uses theACE_UNUSED_ARGandACE_NOTSUP_RETURNmacros for consistent error reporting.ACE/ace/CDR_Stream.cpp (3)
1710-1721: Good defensive programming with memory allocationAdding exception handling here is a good practice to gracefully handle memory allocation failures when resizing the string. This makes the code more robust on platforms where exceptions are supported.
1770-1781: Consistent error handling for memory allocationGood implementation of memory allocation error handling for wstring, matching the pattern used in the string handling implementation and maintaining consistency throughout the codebase.
1790-1801: Proper exception handling for the second wstring resize caseThe exception handling is correctly implemented in this second case where the wstring is resized in a different code path. This ensures consistent behavior regardless of which branch the code follows.
ACE/ace/OS_NS_fcntl.cpp (2)
109-114: Clear handling for systems that lack open supportGood implementation for systems where open is not available. The code properly marks all arguments as unused and returns an appropriate error code.
116-119: Simplified file opening for INTEGRITY178BThe specialized handling for INTEGRITY178B provides a simpler and more direct path to the underlying open call, which is good for maintainability.
ACE/ace/Service_Config.cpp (1)
302-306: Simplified signal handling initializationRemoving the complex signal handler registration while keeping the reactor initialization is a good simplification. This reduces platform-specific code and potential issues with signal handling, which is particularly beneficial for the Green Hills INTEGRITY RTOS.
ACE/ace/OS_NS_sys_utsname.cpp (3)
217-225: Robust error handling for gethostname functionalityGood defensive programming by adding a check for systems that lack gethostname and validating the input parameter. These changes prevent potential crashes or undefined behavior.
226-232: Updated system identification for INTEGRITY-178BProper support for INTEGRITY-178B with specific version information. This enhances platform detection capabilities and provides accurate system information.
233-237: Updated version information for standard INTEGRITYThe version update for standard INTEGRITY ensures the system information remains current and accurate. Using a consistent approach for both INTEGRITY variations makes the code more maintainable.
ACE/ace/CDR_Stream.inl (2)
371-380: Well-implemented support for std::string_viewThe addition of the
write_string_viewmethod properly handles string views by extracting the length and data pointer. The conditional compilation ensures backward compatibility with environments lacking std::string_view support.
399-406: Consistent implementation of operator<< for string_viewThe operator overload follows the same pattern as the existing string operators, ensuring consistent behavior across different string types.
ACE/ace/CDR_Size.inl (2)
155-164: Good implementation of write_string_view for ACE_SizeCDRThe implementation correctly handles std::string_view by using its size() and data() methods, properly checking for empty views. Matches the implementation pattern used in ACE_OutputCDR.
413-420: Consistent operator<< overload for string_viewThe operator implementation correctly delegates to the write_string_view method, maintaining the same pattern as other operator<< overloads in this class.
ACE/ace/config-integrity-common.h (1)
1-182: Comprehensive configuration for INTEGRITY RTOSThis new file provides a thorough set of configuration macros for the INTEGRITY operating system. The implementation includes:
- Detailed specification of missing features via ACE_LACKS_* macros
- Proper definition of available features via ACE_HAS_* macros
- Thread priority settings customized for INTEGRITY
- Appropriate header inclusions and type definitions
The conditional section for ACE_NO_GHS_LIBIO (lines 158-178) provides good flexibility for environments without the INTEGRITY-178 Low-Level I/O Library.
ACE/ace/config-macros.h (2)
523-523: Updated comment for INTEGRITY supportThe comment now correctly references INTEGRITY instead of pSoS, consistent with the PR's objective of updating INTEGRITY RTOS support.
534-539: Added INTEGRITY-specific thread function return typeThe implementation defines ACE_THR_FUNC_RETURN as int for INTEGRITY, with clear documentation explaining why this is needed. The comment explains how this is used by ACE's internal thread adapter function but ignored by the actual entry point passed to INTEGRITY's Task calls.
ACE/ace/CDR_Stream.h (3)
60-63: Provide tests for std::string_view inclusion.Including
<string_view>and conditionally compiling it under#if !defined(ACE_LACKS_STD_STRING_VIEW)is a good approach. To ensure robust integration, consider adding tests (perhaps in a dedicated test suite) that confirm this header is properly found and that all string-view related functionality compiles and runs in both supported and fallback scenarios.
291-293: Confirm correct usage and implementation of write_string_view.The newly introduced
write_string_viewmethod is guarded by#if !defined (ACE_LACKS_STD_STRING_VIEW), which is consistent with the new conditional compilation approach. Ensure you have a corresponding definition and usage tests. It would be prudent to confirm no overlap or ambiguity arises with existing write methods forstd::string.
1447-1450: Evaluate potential overload conflicts for operator<<(std::string_view).Adding an operator<< for
std::string_viewis valuable for modern C++ usage. However, check for potential overload ambiguities if there's already an operator<< forstd::string. Verify that calls with literals or other string-like types do not inadvertently resolve to the wrong overload.ACE/ace/OS_NS_signal.inl (6)
73-74: Clarify INTEGRITY and ACE_LACKS_POSIX handling for sigaction.The fallback path returns with
ACE_NOTSUP_RETURN(-1)when bothINTEGRITYandACE_LACKS_POSIXare defined. This is consistent with a platform lacking complete POSIX signal APIs. If the macro detection has edge cases, consider documenting them more explicitly to avoid confusion.
88-93: Consistent fallback for ACE_LACKS_SIGADDSET.Returning -1 with not supported is consistent. Just ensure that all callers handle the -1 return path gracefully, especially if signals are crucial for the calling logic.
114-118: Consistent fallback for ACE_LACKS_SIGDELSET.Same pattern of skipping functionality and returning -1. Confirm that is the intended design and that no caller expects partial behavior.
139-142: Consistent fallback for ACE_LACKS_SIGEMPTYSET.Again, returning -1 for unsupported. Ensure test coverage exists for these edge macros.
158-161: Consistent fallback for ACE_LACKS_SIGFILLSET.Behavior matches the other signal-set macros. No immediate concerns; maintain a uniform approach throughout the codebase.
177-181: Consistent fallback for ACE_LACKS_SIGISMEMBER.Returning -1 is consistent with the pattern. This completes the granularity of checking each individual signal-set function.
ACE/ace/OS_NS_arpa_inet.inl (1)
55-67: Validate coverage for inet_ntoa fallback case.The conditional block
#if !(defined (INTEGRITY) && defined (ACE_LACKS_INET_NTOA))ensures standardinet_ntoausage if available. Ensure that any custom fallback implementation inACE/ace/OS_NS_arpa_inet.cppis exercised by platform-specific tests, especially on INTEGRITY.ACE/ace/OS_NS_unistd.inl (3)
1048-1050: Special case handling for INTEGRITY's sysconf implementation.The addition of a specific INTEGRITY case for sysconf is a good improvement that returns a predefined page size rather than an error code, making the platform more usable.
957-967: Enhanced sleep() implementation for platforms lacking select.Good enhancement to properly handle platforms where select() is not available by marking arguments as unused and returning the appropriate error code.
Also applies to: 969-977
112-115: Consistent error handling for unsupported operations.The changes follow a consistent and robust pattern for handling unsupported operations across multiple functions (chdir, rmdir, close, ftruncate, getcwd, lseek, read, write). This approach properly marks unused arguments and returns appropriate error codes, improving platform compatibility.
Also applies to: 137-140, 172-174, 375-376, 387-390, 666-670, 717-721, 1192-1196
ACE/ace/config-integrity.h (3)
6-74: Well-structured configuration for INTEGRITY RTOS support.This configuration file appropriately identifies POSIX limitations in the INTEGRITY environment and provides necessary workarounds. The comprehensive list of ACE_LACKS_* macros aligns with the error handling changes in implementation files, creating a cohesive approach to INTEGRITY support.
19-23: Intentional TSS emulation for compatibility with INTEGRITY-178.Good design decision to use thread-specific storage emulation even though INTEGRITY supports TSS natively. The comment clearly explains this is for compatibility with INTEGRITY-178, which helps future maintainers understand this deliberate choice.
12-15: Effective workaround for UserException name conflict.The ORBEXPRESS macro definition prevents header conflicts with INTEGRITY's UserException, showing attention to detail in handling integration challenges between the frameworks.
ACE/ace/OS_NS_sys_stat.inl (1)
22-25: Consistent error handling for unsupported file operations.The changes follow the same robust pattern seen in other files, marking unused arguments and returning appropriate error codes for unsupported operations. This enhances platform compatibility while maintaining clean code that won't generate compiler warnings.
Also applies to: 68-69, 155-157, 203-204
TAO/tao/Strategies/advanced_resource.cpp (3)
28-28: Header inclusion for new reactor implementation.The header inclusion for Single_Input_Reactor follows the same pattern as other reactor headers, maintaining consistency in the codebase.
200-201: Command-line option for the new Single_Input_Reactor.The command-line argument handling for "single_input" follows the same pattern as other reactor types, ensuring consistency in how the application is configured.
479-481:✅ Verification successful
Simple instantiation of the new reactor.
The instantiation of ACE_Single_Input_Reactor is notably simpler than other reactors, which take several arguments. Verify that this simpler instantiation is intentional and whether additional configuration parameters should be passed.
🏁 Script executed:
#!/bin/bash # Check if ACE_Single_Input_Reactor constructor takes parameters grep -A 10 "ACE_Single_Input_Reactor::" ACE/ace/Single_Input_Reactor.cpp | grep -E "ACE_Single_Input_Reactor\s*\("Length of output: 114
I'll generate an additional verification script to search for the constructor definition of
ACE_Single_Input_Reactoracross both header and source files to ensure we capture any signature with parameters. This will help us confirm whether the simple instantiation is intentional.
🏁 Script executed:
#!/bin/bash echo "Searching for the constructor definition in cpp files:" grep -R "ACE_Single_Input_Reactor::ACE_Single_Input_Reactor(" -n . echo "Searching for the class declaration in header files:" grep -R "class ACE_Single_Input_Reactor" -n .Length of output: 445
Verification Outcome: Minimal Instantiation Confirmed
After further investigation, the search for a parameterized constructor in both source and header files revealed that
ACE_Single_Input_Reactoris declared in the header without any constructor signature indicating additional parameters. This suggests that its simple instantiation inadvanced_resource.cppis intentional. Ensure that any reactor-specific configuration that might be required is handled elsewhere (if applicable).ACE/ace/Flag_Manip.cpp (2)
60-79: Conditional logic for non-blocking on GHS simulator is consistent.
WhenACE_USES_GHS_ISIMPPCis defined, the code short-circuits for theACE_NONBLOCKflag and otherwise delegates to the helper function when available. This approach is consistent with the simulator’s known limitations. The switch statement is concise and aligns with existing patterns in ACE for platform-specific overrides.
116-129: Consistent conditional handling for clearing flags.
Similar to theset_flagsapproach, returning 0 forACE_NONBLOCKunderACE_USES_GHS_ISIMPPCkeeps the simulator happy while other flags route toclr_flags_helper(). This maintains consistency across the code and prevents attempts to clear unsupported flags.ACE/ace/config-integrity178.h (4)
1-19: Header guard and RTOS version macro introduced successfully.
DefiningINTEGRITY178Band includingconfig-integrity-common.hhere is a suitable approach for distinguishing this specialized variant of INTEGRITY. Ensure all dependent build scripts and project files properly reference this new header to activate the correct macros.
23-73: Extensive POSIX omission macros.
By grouping many “LACKS” macros under#if defined ACE_LACKS_POSIX, you accurately communicate the features unavailable in INTEGRITY-178B. However, a large number of omitted calls may hide partially supported features. If partial support evolves, consider splitting these definitions or verifying them more granularly to reduce potential confusion down the road.
75-106: Selective network stack exclusion.
UnderACE_NO_GHNET178_LIBNET, you’re correctly disabling higher-level network utilities while retaining basic socket functions throughlibsocket.a. This helps keep the build minimal on resource-constrained RTOS targets. Ensure that any TAO or ACE features requiring these omitted utilities are conditionally excluded to avoid linker errors or unresolved symbols.
108-116: C++17 and POSIX feature macros.
Marking outACE_LACKS_STD_STRING_VIEWaligns with the known limitations of this platform’s standard library. Double-check other C++17 capabilities likestd::optionalorstd::filesystemif you plan on using them in the future, as they might also be missing.ACE/ace/config-integritySCA.h (4)
3-16: Updated header guard and RTOS version reference.
Renaming the guard toACE_CONFIG_INTEGRITYSCA_Hand updating references to version 11.4.x clarifies the intended environment. This reduces confusion for users migrating from olderACE_INT_CONFIG_Hreferences. The new comments effectively convey that POSIX APIs are available.
35-50: Exception and standard library features.
Enabling C++ exception support (ACE_HAS_EXCEPTIONS) and the standard library ensures broader compatibility with modern C++ patterns. Confirm that the GHS compiler for this INTEGRITY version also supports these exception features thoroughly, as partial exceptions can lead to unexpected behavior.
190-191: Namespace usage and RCSID toggling.
DefiningACE_USES_STD_NAMESPACE_FOR_STDCPP_LIBand disablingACE_USE_RCSIDis a standard housekeeping measure. There should not be any adverse effects, but verify that third-party code using RCS strings or relying on non-namespace calls is unaffected.
228-228: End of header guard.
Everything is properly closed out with#endif. No concerns here.ACE/ace/OS_NS_Thread.h (3)
233-267: Definitions for INTEGRITY look consistent.
These type and macro definitions cleanly extend thread abstractions to INTEGRITY. The code is logically consistent, and using a counting semaphore initialized with 1 as a mutex aligns with other ACE patterns on platforms lacking dedicated mutex APIs.
268-273: Forward declarations are straightforward.
Forward declaringthread_mutex_lockandthread_mutex_unlockinsideACE_OSis clear. Please confirm that the definitions in the corresponding.cppfile match these signatures.
274-289: RAII LockGuard implementation is good.
This simple struct properly enforces a lock/unlock sequence. Consider adding error checking ifthread_mutex_lockcan fail, but if the environment guarantees success, this is fine.ACE/ace/OS_NS_Thread.cpp (13)
204-210: Use of named mutex guard improves clarity.
The newly introducedguardvariable clarifies the scope of the lock and makes the code more readable. No functional issues found.
215-220: Consistent naming for recursive mutex guard.
Same improvement as above: readability is enhanced with a named guard. Everything looks good.
256-260: Minor clarity improvement by naming the guard.
The change to a namedACE_OS_Recursive_Thread_Mutex_Guardinstance is consistent with the preceding segments. No concerns noted.
273-277: No issues with guard instantiation.
Naming the guard here, too, seems consistent with the approach in the other TSS Emulation methods.
1154-1158: Adding INTEGRITY to the conditional block.
Extending the existing#if defined (ACE_VXWORKS) || defined (ACE_MQX)condition to includeINTEGRITYlooks correct. Ensures the same semaphore-based flow is used.
1178-1180: Condition variable destruction logic extended for INTEGRITY.
IncludingINTEGRITYin the#elifensures proper handling incond_destroy. The code is consistent with the other platforms.
1228-1230: INTEGRITY condition-object initialization check.
Similarly includingINTEGRITYin thecond_initpath forwaiters_done_respects the existing pattern for VxWorks and MQX.
1258-1260: Wide-char condition init now covers INTEGRITY.
This mirrors the logic in the non-wide version and is consistent with the approach for other platforms.
1925-1934: mutex_init: Introducing INTEGRITY branch.
The new branch for INTEGRITY is consistent with the existing pattern for VxWorks. Ensuring the correct semaphore or handle is created looks fine.
1975-1976: mutex_destroy: INTEGRITY branch calling sema_destroy.
Destroying the semaphore for the INTEGRITY path is consistent with the rest of the code. Change looks straightforward.
2070-2072: mutex_lock for INTEGRITY uses WaitForSemaphore.
This mirrors the approach taken for VxWorks (semTake). Implementation appears correct.
3925-4057: thr_create logic for INTEGRITY and INTEGRITY-178B.
The custom calls (e.g.,SetupTaskvs.CommonCreateTask) and dynamic stack usage are valid approaches. Just ensure the deallocation path is robust for tasks that exit unexpectedly and that stack management is thoroughly tested.Please verify that tasks created this way are properly cleaned up under all exit conditions (normal and abnormal).
4221-4282: thr_join for INTEGRITY.
This implementation uses a global map of semaphores to sync with the joined thread’s exit. The logic is coherent. No immediate concerns beyond ensuring the map is pruned properly on all exit paths.ACE/ace/OS_NS_Thread.inl (8)
1-1: Cppcheck unknown macro warning.The static analysis tool reports an unknown macro for
ACE_BEGIN_VERSIONED_NAMESPACE_DECL. This is a known ACE macro. No functional code change is required; please adjust the static analysis configuration to recognize it.
23-25: Clarify conditional usage for INTEGRITY TSS.These lines add a specific conditional block for the INTEGRITY OS when using TSS emulation. Ensure that all references to
ACE_INTEGRITY_TSS_Implare linked in the build and that the code is tested for multi-thread scenarios, as TSS typically involves concurrency complexities.
54-57: Consider verifying pointer validity.When retrieving
ACE_INTEGRITY_TSS_Impl* obj, confirm it is not null before dereferencing. IfACE_INTEGRITY_TSS_IMPLwere not allocated, accessingobj->get()could lead to undefined behavior.
1749-1756: Check for concurrency edge cases.Calling
::CreateSemaphore(count, s)sets up the semaphore on INTEGRITY. Ensure the system call’s parameters align with intended concurrency semantics, including the maximum possible count and the initial count. Otherwise, resource limits may be unintentionally exceeded or underutilized.
1894-1895: Confirm ReleaseSemaphore usage.When using
::ReleaseSemaphore(*s), verify that its return valueSuccessconsistently denotes full success. Ensure the function’s behavior matches typical semaphore post semantics, particularly regarding potential error conditions.
2000-2001: Validate trylock semantics.In
sema_trywait(), returning0forSuccessor-1otherwise should match a non-blocking semaphore acquisition. Confirm it aligns with the broader ACE expectations on a “try” operation.
2555-2560: Ensure priority retrieval logic is robust.The
GetActivePriority()call might be subject to range constraints. Ensure that special or idle threads cannot produce unexpected values, and confirm no off-by-one scenarios arise for maximum or minimum priorities.
3172-3173: Verify thread halt approach.
::HaltTask(target_thread)may be irreversible, effectively killing the thread. If the intent is a suspend/resume model, investigate whether INTEGRITY offers a different call for true suspension.ACE/ace/Single_Input_Reactor.cpp (1)
22-35: Validate handle registration.
register_handlersimply overwritesevent_handler_and disables non-blocking. Confirm there is no existing handler to prevent overwriting, and ensure that the handle is valid. Also, consider the lifecycle if multiple registrations inadvertently occur without removal.ACE/ace/Single_Input_Reactor.h (1)
13-67: Review placeholders in reactor methods.Most methods return a default value or 0. Confirm they won't be unexpectedly called by other ACE/TAO components (e.g., timers, signals), as it could yield silent failures or missing functionality.
|
Since #2339 was recently merged, you may want an explicit merge from the master branch to pick up the updates to the CI configuration |
TAO/tao/default_resource.cpp
Outdated
| @@ -791,12 +791,14 @@ TAO_Default_Resource_Factory::get_reactor () | |||
| else | |||
| this->dynamically_allocated_reactor_ = true; | |||
|
|
|||
| ACE_Reactor::instance (reactor); | |||
There was a problem hiding this comment.
Doesn't look a good thing to just replace the singleton reactor with the one created by TAO, something to revert, also reclaim
There was a problem hiding this comment.
Looks like that to me. I think this was a temporary solution to enforce the use of the new ACE_Single_Input_Reactor everywhere @mitza-oci
There was a problem hiding this comment.
It will need some more analysis. The problem is that parts of TAO were depending on ACE's singleton reactor instead of using the ORB's reactor.
There was a problem hiding this comment.
Maybe TAO_DEFAULT_REACTOR_TYPE is a helpful define to change the default reactor type for TAO?
There was a problem hiding this comment.
Or extend
Line 52 in b880144
There was a problem hiding this comment.
TAO allows, or seems to allow, a different reactor to be configured for the ORB, separate from ACE's singleton reactor or any other reactor.
There was a problem hiding this comment.
Yes, but the code here still resets it, so something to remove
There was a problem hiding this comment.
We're working on it -- see #2340 (comment)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
ACE/ace/config-integrity-common.h (2)
4-5: Question the necessity of definingghsmacro explicitly.Defining a short macro like
ghscan cause name clashes and confusion unless it is strictly required for conditional compilation. If this macro is only used internally to signify Green Hills Software environment, consider using a more descriptive name (e.g.,ACE_GHS_ENV) or conditionally defining it via a compiler flag.
108-111: Clarify build instructions forACE_INTEGRITY_NO_EXCEPT.The comment suggests modifying
config.hto build with--no-exceptions. Ensure that the build system or documentation is updated to explain how to set this macro and the consequences of disabling exceptions on ACE functionalities.ACE/ace/Single_Input_Reactor.cpp (5)
12-16: Return type mismatch ininitialized().This method returns
boolbut always returns1. Although that is valid, consider returningtruefor clarity.-bool ACE_Single_Input_Reactor::initialized () -{ - return 1; -} +bool ACE_Single_Input_Reactor::initialized () +{ + return true; +}
17-21: Suspicious usage of boolean expression in asize_treturn type.This method returns
event_handler_.handler() != nullptrwhich is effectively0or1. If you intend to convey the number of registered handlers, consider returningevent_handler_.handler() ? 1 : 0with a proper comment, or expand this logic if multiple handlers ever become supported.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 21-21: There is an unknown macro here somewhere. Configuration is required. If ACE_BEGIN_VERSIONED_NAMESPACE_DECL is a macro then please configure it.
(unknownMacro)
37-44: Handle return codes and potential errors fromhandle_input().Currently, the return value from
handler->handle_input()is ignored. In some scenarios, the event handler may return error codes that indicate disconnection, partial reads, or other conditions. Consider checking this return code to decide whether to remove the handler, log an error, or perform any cleanup.
46-50: Confirm thread-safety requirements oflock().Returning a static
ACE_Lock_Adapter<ACE_Null_Mutex>implies that no real synchronization is performed on the reactor operations. If thread-safety is required (as indicated byACE_MT_SAFEin other configs), ensure that concurrent access is handled safely elsewhere.
52-343: Acknowledge the no-op stubs for multi-handler or multi-event scenarios.All other methods (e.g., remove_handler, suspend_handler, resume_handler, timer_* methods) return 0 or do nothing. While acceptable for the single-input use-case, keep in mind that expansions (e.g., timeouts, multiple event handlers, signals) will require additional implementation. Follow the
ACE_Reactor_Implcontract carefully to avoid unexpected runtime behaviors.ACE/ace/Single_Input_Reactor.h (1)
3-11: Provide clarity on limitations and intended usage.The documentation states the limitations (only one input, no write/exception event handling, no timeouts or notifications). Emphasize these constraints in the header comments, possibly with usage examples, to help users avoid mistakenly using this reactor for unsupported scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.gitignore(2 hunks)ACE/ace/Single_Input_Reactor.cpp(1 hunks)ACE/ace/Single_Input_Reactor.h(1 hunks)ACE/ace/ace.mpc(1 hunks)ACE/ace/config-integrity-common.h(1 hunks)ACE/ace/config-integritySCA.h(7 hunks)ACE/tests/Dynamic_Priority_Test.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- .gitignore
- ACE/ace/ace.mpc
- ACE/tests/Dynamic_Priority_Test.cpp
- ACE/ace/config-integritySCA.h
🧰 Additional context used
🪛 Cppcheck (2.10-2)
ACE/ace/Single_Input_Reactor.cpp
[error] 21-21: There is an unknown macro here somewhere. Configuration is required. If ACE_BEGIN_VERSIONED_NAMESPACE_DECL is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: ubuntu-24.04 g++-10 ACE for TAO
- GitHub Check: ubuntu-24.04 clang++-15
- GitHub Check: ubuntu-24.04 clang++-14
- GitHub Check: ubuntu-22.04 clang++-13
- GitHub Check: ubuntu-22.04 clang++-12
- GitHub Check: VS2022Release64
- GitHub Check: VS2022Debug64
- GitHub Check: VS2022WChar
- GitHub Check: VS2019Debug64Cxx20
- GitHub Check: VS2019Debug64Cxx17
- GitHub Check: VS2019Release32
- GitHub Check: VS2019Debug32
- GitHub Check: VS2019Release64
- GitHub Check: VS2019Debug64
- GitHub Check: VS2019WChar
- GitHub Check: macos-14-C++
- GitHub Check: MINGW64-gcc
- GitHub Check: macos-13-C++
- GitHub Check: alpine-3.18
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
ACE/ace/config-integrity-common.h (2)
19-27: Ensure compatibility of C++ feature macros across compilers.Macros like
ACE_HAS_STANDARD_CPP_LIBRARY,ACE_HAS_TEMPLATE_SPECIALIZATION, andACE_HAS_TYPENAME_KEYWORDcan vary by compiler version and standard. Confirm that these definitions accurately reflect the capabilities of the target INTEGRITY compiler and standard library.
146-148: Assess the potential collision or duplication forunlinkdeclaration.The explicit extern "C" declaration for
int unlink(const char*);may overlap with standard library declarations. Confirm that this does not conflict with<unistd.h>or other headers. If the function is already prototyped, consider guarding it with an#ifndefcheck.ACE/ace/Single_Input_Reactor.cpp (1)
22-35: Carefully manage reference counting for the single event handler.After calling
event_handler->add_reference(), ensure there is a matching decrement or release of this reference during removal or destruction to avoid leaks. Verify thatremove_handleror the destructor callsremove_reference()on the same object if needed.ACE/ace/Single_Input_Reactor.h (1)
166-167: Ensure destructor or unregistration logic forevent_handler_.While
ACE_Event_Handler_varreferences are typically safe, confirm that there is an eventual cleanup path if the reactor object is destroyed, especially ifadd_reference()was done. If the destructor is in the.cppfile, it should finalize references properly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
ACE/ace/OS_NS_Thread.h (1)
268-289: Consider handling lock failures within LockGuard.
IfACE_OS::thread_mutex_lockorACE_OS::thread_mutex_unlockfails (for instance, due to resource exhaustion), the current implementation silently proceeds without error handling. Logging or at least returning an error state could prevent silent failures.ACE/ace/OS_NS_Thread.cpp (3)
1567-1573: Return-1for unsupported timed wait under INTEGRITY
Currently,cond_timedwaitis not implemented for INTEGRITY-178. If partial support becomes available, consider updating this code.I can open an issue to investigate partial timed-wait emulation if desired.
3300-3445: Introduce global data for INTEGRITY-178 Task argument management
Defining a global map and semaphore for each task helps simulate per-thread arguments. However, consider verifying potential leaks if tasks exit unexpectedly without cleanup. A safer approach might be a reference-counted or RAII-based data structure to guarantee resource release.
3995-4059: Override user priority for INTEGRITY
The code forcibly uses the maximum parent priority to comply with INTEGRITY-178 constraints, ignoring the user-provided priority. Documenting this silently ignored parameter avoids confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ACE/ace/OS_NS_Thread.cpp(19 hunks)ACE/ace/OS_NS_Thread.h(3 hunks)ACE/ace/ace_for_tao.mpc(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: VS2022Debug64
- GitHub Check: VS2019Release64
- GitHub Check: VS2019Release32
- GitHub Check: VS2019Debug64Cxx20
- GitHub Check: VS2022WChar
- GitHub Check: VS2022Release64
- GitHub Check: VS2019Debug64
- GitHub Check: VS2019Debug32
- GitHub Check: VS2019Debug64Cxx17
- GitHub Check: VS2019WChar
- GitHub Check: ubuntu-22.04 clang++-13
- GitHub Check: MINGW64-gcc
- GitHub Check: ubuntu-22.04 clang++-12
- GitHub Check: ubuntu-24.04 g++-12
- GitHub Check: macos-14-C++
- GitHub Check: ubuntu-24.04 g++
- GitHub Check: macos-13-C++
- GitHub Check: windows-2019 vs2019
- GitHub Check: alpine-3.18
🔇 Additional comments (14)
ACE/ace/ace_for_tao.mpc (2)
155-155:✅ Verification successful
Addition of Single_Input_Reactor.cpp to ACE_FOR_TAO project.
The inclusion of
Single_Input_Reactor.cppis aligned with the PR objectives of enhancing support for Green Hills INTEGRITY and INTEGRITY-178 RTOS. This reactor implementation likely provides specialized event handling capabilities required for these RTOS environments.Let's verify that this addition is properly integrated with the rest of the codebase:
🏁 Script executed:
#!/bin/bash # Description: Check for the existence of Single_Input_Reactor header file and any references in the codebase # Check if the corresponding header file exists echo "Checking for header file:" fd "Single_Input_Reactor.h" --type f # Check for references to Single_Input_Reactor in the codebase echo -e "\nReferences to Single_Input_Reactor in the codebase:" rg "Single_Input_Reactor" --type cpp -A 2 -B 2Length of output: 18265
Integration of Single_Input_Reactor verified
The addition of
Single_Input_Reactor.cppis properly integrated with the codebase. Our verification confirms that:
- The corresponding header file (
ACE/ace/Single_Input_Reactor.h) exists.- References to the new reactor are present in the TAO implementation (e.g., in
TAO/tao/Strategies/advanced_resource.cpp).This implementation aligns with the PR objectives for supporting Green Hills INTEGRITY and INTEGRITY-178 RTOS.
1-9:❓ Verification inconclusive
Keep in mind the project's design constraints.
The file comments emphasize that the ACE_FOR_TAO subset is designed primarily for the TAO CosNotification service and must remain as small as possible. While adding the Single_Input_Reactor component appears justified for INTEGRITY RTOS support, ensure that this addition directly supports the core functionality and doesn't unnecessarily increase the library size.
Let's verify that this component is necessary for the INTEGRITY RTOS support:
🏁 Script executed:
#!/bin/bash # Description: Check for INTEGRITY-related functionality in Single_Input_Reactor implementation # Look for INTEGRITY-related conditionals or references in the Single_Input_Reactor implementation echo "Checking for INTEGRITY references in Single_Input_Reactor implementation:" fd "Single_Input_Reactor.cpp" --type f --exec grep -i -A 3 -B 3 "integrity" {} \; # Check if Single_Input_Reactor is referenced in any INTEGRITY-specific code echo -e "\nChecking for Single_Input_Reactor usage in INTEGRITY-specific code:" rg -i "integrity" --type cpp -A 5 -B 5 | rg "Single_Input_Reactor"Length of output: 455
Attention: Verify the Necessity of Single_Input_Reactor for INTEGRITY RTOS Support
The recent checks did not reveal any explicit INTEGRITY references or usage in the Single_Input_Reactor implementation. Given the project's design constraint—to keep the ACE_FOR_TAO subset minimal and focused on TAO CosNotification—the addition of Single_Input_Reactor should be scrutinized. Please manually verify that its inclusion is essential for INTEGRITY RTOS support and strictly aligns with core functionality requirements.
- No explicit INTEGRITY-related conditionals or usage were found in Single_Input_Reactor.cpp.
- Ensure that any integration points involving this component clearly justify its inclusion without unnecessarily expanding the subset.
ACE/ace/OS_NS_Thread.h (3)
43-47: No issues found in these added includes for INTEGRITY support.
Everything appears correct, with minimal risk of conflicts or definition collisions.
233-248: Confirm the appropriateness of using counting semaphores as thread mutexes.
While using a counting semaphore with an initial count of 1 effectively behaves like a mutex, ensure it won't introduce edge cases (e.g., repeated locks by the same thread). Verify that this design aligns with INTEGRITY’s concurrency model.
249-289: Validate that zero-valued macro definitions meet system & library expectations.
Defining all thread-related macros to 0 (e.g., THR_CANCEL_DISABLE, THR_BOUND, etc.) may produce unintended results if any higher-level logic expects non-zero flags for fine-grained concurrency. Confirm none of these macros are interpreted in a way that breaks thread behaviors on INTEGRITY.ACE/ace/OS_NS_Thread.cpp (9)
1154-1158: Add INTEGRITY condition for broadcast completion handling
These lines consistently apply the semaphore-based approach for INTEGRITY incond_broadcast, matching the existing logic for VxWorks and MQX. The added check looks correct.
1178-1180: Include INTEGRITY incond_destroybranch
Extending the semaphore destruction path to INTEGRITY appears aligned with the approach for VxWorks and MQX.
1228-1230: Initializewaiters_done_via semaphores for INTEGRITY
This mirrors the pattern used by VxWorks and MQX and is logically consistent with the added INTEGRITY condition.
1258-1260: Parallel path for INTEGRITY incond_init
Creating thewaiters_done_semaphore under INTEGRITY follows the same approach as for other RTOS definitions.
1390-1394: Post semaphore incond_waitfor INTEGRITY
Postingwaiters_done_is consistent with how the code handles VxWorks and MQX conditions for the last waiter scenario.
1925-1933: Add directCreateSemaphorecall for INTEGRITY
Replacing the VxWorkssemMCreateapproach withCreateSemaphoreunder INTEGRITY is consistent. Ensure that error handling (for non-success returns) is tested.
2070-2072: AdoptWaitForSemaphorefor mutex locking
Use ofWaitForSemaphore(*m)here matches the forever-wait semantics ofsemTakefor VxWorks.
2331-2347: Potential concurrency race inmutex_unlockfor INTEGRITY
This logic mirrors the previously flagged concurrency concern where multiple threads calling unlock can interleave calls toGetSemaphoreValueandReleaseSemaphore.
4224-4284: Newthr_joinimplementation for INTEGRITY
Storing a semaphore per task to synchronize join calls is a sensible approach. Remain mindful of potential edge cases if a task never callsthr_join. Validate that the intermediate mapping entries are properly erased to prevent indefinite map growth.
|
I have updated the porting documentation, see #2344 |
TAO/tao/default_resource.cpp
Outdated
| @@ -791,12 +791,14 @@ TAO_Default_Resource_Factory::get_reactor () | |||
| else | |||
| this->dynamically_allocated_reactor_ = true; | |||
|
|
|||
| ACE_Reactor::instance (reactor); | |||
There was a problem hiding this comment.
It will need some more analysis. The problem is that parts of TAO were depending on ACE's singleton reactor instead of using the ORB's reactor.
| #define ACE_LACKS_SETEUID | ||
| #define ACE_LACKS_GETGID | ||
|
|
||
| #define _REENTRANT |
There was a problem hiding this comment.
Shouldn't this come from the OS headers?
|
Add news items |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
ACE/ace/OS_NS_Thread.h (1)
336-341: 🛠️ Refactor suggestionHandle error paths more gracefully in TSS emulation.
Currently, when
ACE_MAX_NUM_THREADSis exhausted, the code callsACE_OS::exit(EXIT_FAILURE), which terminates the entire process. This is a drastic measure that might not be appropriate for all applications. Consider returning an error code or implementing a fallback strategy to allow for controlled recovery from this situation.if (first_slot == ACE_MAX_NUM_THREADS) { - ACE_OS::exit (EXIT_FAILURE); + // Return an error code instead of terminating the process + // Consider: setting a status flag, throwing an exception, or using a fallback slot + return nullptr; // Or appropriate error representation }ACE/ace/OS_NS_Thread.cpp (1)
1930-1937:⚠️ Potential issueImplementing mutex with a counting semaphore for INTEGRITY.
Be aware that multiple simultaneous unlock calls could attempt to increment the semaphore beyond 1, leading to a race condition. Use strict ownership checks or an atomic approach if this scenario is expected.- // Make sure the semaphore's value does not exceed its initial value, which is 1. - // This guards against incorrect unlocks from threads without holding the lock. - // ... - if (curr_val == 1) - { - errno = EPERM; - return -1; - } - return ::ReleaseSemaphore(*m) == Success ? 0 : -1; + // Optionally use system-provided mutex (if available) or add an atomic check. + // E.g., a robust check that prevents increments beyond 1 from two unlocking threads.
🧹 Nitpick comments (2)
TAO/docs/Options.html (1)
1514-1519: Document the newsingle_inputreactor option clearly.The added table row explains that the
ACE_Single_Input_Reactorcan be used on systems that lack an I/O de-multiplexing function (e.g., usingselect). Verify that this description accurately reflects the behavior of the reactor implementation and that it is placed in the proper section of the Advanced Resource Factory options. Consider adding a brief note regarding any platform-specific requirements or build flags, if applicable, to further guide users.ACE/ace/OS_NS_Thread.cpp (1)
1571-1577: No timed wait implementation for INTEGRITY here.
You return-1withACE_NOTSUP_RETURNfor timed waits on INTEGRITY. If future integration of timeouts is possible, consider implementing or clearly documenting this limitation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
ACE/NEWS(1 hunks)ACE/ace/OS_NS_Thread.cpp(20 hunks)ACE/ace/OS_NS_Thread.h(2 hunks)ACE/ace/OS_NS_fcntl.cpp(1 hunks)ACE/ace/OS_NS_sys_socket.inl(3 hunks)ACE/ace/OS_NS_sys_utsname.cpp(1 hunks)ACE/ace/OS_NS_unistd.inl(11 hunks)ACE/ace/config-integrity-common.h(1 hunks)ACE/ace/config-integritySCA.h(6 hunks)TAO/docs/Options.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- ACE/ace/OS_NS_sys_socket.inl
- ACE/ace/OS_NS_unistd.inl
- ACE/ace/OS_NS_sys_utsname.cpp
- ACE/ace/config-integritySCA.h
- ACE/ace/config-integrity-common.h
🧰 Additional context used
🧬 Code Definitions (2)
ACE/ace/OS_NS_Thread.h (3)
ACE/ace/OS_NS_Thread.cpp (6)
lock(2385-2392)lock(2385-2385)id(3377-3381)id(3377-3377)id(3385-3398)id(3385-3385)ACE/ace/Single_Input_Reactor.cpp (2)
lock(47-51)lock(47-47)ACE/ace/OS_NS_stdlib.cpp (2)
exit(43-63)exit(44-44)
ACE/ace/OS_NS_Thread.cpp (1)
ACE/ace/OS_NS_Thread.h (15)
ACE_OS_Recursive_Thread_Mutex_Guard(2079-2079)ACE_OS_Recursive_Thread_Mutex_Guard(2082-2082)ACE_OS_Recursive_Thread_Mutex_Guard(2098-2098)guard(314-314)guard(353-353)id(312-347)id(312-312)id(351-364)id(351-351)id(752-752)id(755-755)id(758-758)thr_id(1777-1777)thr_join(1677-1677)thr_join(1684-1686)
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: macos-14-C++
- GitHub Check: macos-13-C++
- GitHub Check: ubuntu-24.04 g++-10 CodeQL
- GitHub Check: ubuntu-24.04 g++-13
- GitHub Check: ubuntu-24.04 g++-14
- GitHub Check: ubuntu-22.04 clang++-13
- GitHub Check: ubuntu-24.04 g++-12
- GitHub Check: ubuntu-24.04 g++-11
- GitHub Check: ubuntu-22.04 g++-10
- GitHub Check: VS2019Debug64Cxx17
- GitHub Check: VS2022WChar
- GitHub Check: VS2019Debug32
- GitHub Check: VS2019Release64
- GitHub Check: windows-2019 vs2019
- GitHub Check: VS2022Debug64
- GitHub Check: VS2019Debug64
- GitHub Check: VS2019WChar
- GitHub Check: alpine-3.18
- GitHub Check: MINGW64-gcc
- GitHub Check: ubuntu-24.04 g++
🔇 Additional comments (19)
ACE/NEWS (1)
21-22: Good addition to NEWS file.The added entry correctly documents the updated support for Green Hills INTEGRITY and INTEGRITY-178 tuMP RTOS, following the established format of the NEWS file.
ACE/ace/OS_NS_fcntl.cpp (3)
109-114: Appropriate handling for platforms lacking open function.This new code path correctly handles platforms where the open function is not available by marking all arguments as unused and returning INVALID_HANDLE with a "not supported" indication.
115-117: Good addition for INTEGRITY-178 specific handling.This branch provides specialized handling for INTEGRITY-178, ignoring the security attributes parameter (which is not applicable for this platform) and directly calling the system's open function.
118-118: Correctly changed if to elif following new condition.The existing INTEGRITY check is now properly preceded by "elif" after adding the INTEGRITY-178 specific case.
ACE/ace/OS_NS_Thread.h (7)
298-298: Use uniform initialization, as suggested.The code correctly uses uniform initialization syntax which is more consistent with modern C++ practices.
240-244: Evaluate potential limitations of using a semaphore as a mutex.While using a semaphore with an initial count of 1 can emulate a non-recursive mutex, this approach may not support all mutex behaviors. Specifically, it doesn't support recursion and may not detect improper release calls. Ensure this implementation aligns with desired POSIX-congruent behavior and that the code never requires more advanced mutex features.
303-306: Revisit the static array usage for TSS entries.While the array-based approach avoids the previously encountered memory issue with std::map, it caps the number of threads at
ACE_MAX_NUM_THREADS(defaulting to 64 if not defined). In high-concurrency scenarios, this could potentially lead to overflow issues, guarded only byACE_ASSERT. Consider documenting this limitation or exploring alternative dynamic container approaches if the memory issue with std::map can be resolved.
227-262: Thread configuration for INTEGRITY looks appropriate.The thread configuration macros and types for INTEGRITY are properly defined, following the same pattern as other supported platforms in the codebase. The definitions establish the necessary types and constants for thread operations on the INTEGRITY platform.
270-285: Well-implemented RAII pattern for mutex locking.The
LockGuardstructure properly implements the RAII (Resource Acquisition Is Initialization) pattern for mutex locks, ensuring that locks are always released, even in the presence of exceptions. This implementation follows good C++ practices.
312-347: Thread-specific storage implementation is thread-safe.The
getmethod inACE_INTEGRITY_TSS_Implcorrectly uses aLockGuardto maintain thread safety while accessing and potentially modifying the shared TSS array. The implementation correctly checks for existing entries and allocates new ones as necessary, with appropriate error handling.
351-364: Clean implementation of the TSS entry deallocation.The
erasemethod properly handles thread completion by marking entries as invalid, allowing slots to be reused. This implementation is thread-safe due to the use ofLockGuardand correctly implements the required functionality for managing thread-specific storage lifecycle.ACE/ace/OS_NS_Thread.cpp (8)
24-27: Include<map>only if strictly necessary.
Including<map>here is logical for storing thread data, but ensure that using the standard library on INTEGRITY is fully supported for your embedded environment.
1158-1162: Expanding condition to includeACE_INTEGRITY.
This change ensures that conditional logic incond_broadcastalso applies to INTEGRITY.
1182-1182: Add INTEGRITY support forcond_destroy.
This straightforward extension aligns with the existing conditional checks for other platforms.
1232-1232: Extendcond_initto handle INTEGRITY.
This is consistent with the rest of the changes and avoids missing logic for this platform.
1253-1253: Wchar version ofcond_initnow covers INTEGRITY.
No issues spotted, consistency is maintained across overloads.
1393-1393: Include INTEGRITY incond_wait.
Matches the existing pattern for other RTOS condition checks.
1979-1980: Final semaphore destruction for INTEGRITY.
Implementation is consistent with other platform branches, ensuring resources are released.
3304-3445: Global map usage for INTEGRITY tasks and stack management.
Overall logic is sound:
• The global maps track thread adapters and join semaphores, guarded by a single static mutex.
• The pre-allocated stack pool prevents excessive threads beyondNUM_DYNAMIC_THREADS.
• The join logic erases entries after the thread exits, ensuring one join per thread.Potential concerns:
• Multiple calls to thr_join on the same thread are unguarded and may yield EINVAL after the first join.
• For INTEGRITY-178B, the semaphore is never closed, which could lead to resource leaks across restarts.If these behaviors are expected and documented, this approach is fine. Otherwise, consider adding additional safeguards or documentation.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
ACE/ace/OS_NS_Thread.cpp (1)
2340-2349:⚠️ Potential issuePartial fix for race condition when unlocking.
Although the code checksGetSemaphoreValuebeforeReleaseSemaphore, multiple threads unlocking concurrently can still raise the semaphore above 1. Consider wrapping the semaphore usage in more robust ownership tracking or using an actual RTOS mutex API to prevent race conditions.- if (curr_val == 1) - { - errno = EPERM; - return -1; - } - return ::ReleaseSemaphore(*m) == Success ? 0 : -1; + // Proposed approach: verify ownership or store an owner ID + // to enforce correct usage, or rely on a built-in kernel mutex.
🧹 Nitpick comments (20)
ACE/ace/OS_NS_stdlib.cpp (1)
1229-1231: Program name assignment for the entire string
Similar to above, aconst_castis employed to track the program name under ACE_INTEGRITY. Confirm that no code attempts to write into the literal pointed by__progname.If needed, I can propose a safer alternative that copies the string to a local buffer if you anticipate modifications.
ACE/ace/CDR_Stream.cpp (1)
1790-1801: Repeated try/catch block for alternative wstring path.Maintains consistency with the rest of the allocation handling. Consider factoring repeated try/catch logic into a helper function to reduce duplication across the codebase.
ACE/ace/CDR_Stream.h (1)
291-293: Consider adding a short documentation comment forwrite_string_view.
A brief docstring describing how the method handlesstd::string_viewsemantics (e.g., potential zero-termination or length considerations) would improve clarity.ACE/ace/config-integrity.h (1)
60-61: Avoid duplication of NSIG definition.
NSIGis set to(SIGRTMAX+1)both here and inconfig-integritySCA.h. Consider consolidating the definition to a common header or ensuring each file truly needs its own separate definition.ACE/ace/Flag_Manip.cpp (3)
24-58: Consider extra error handling and testing for helper functions.
set_flags_helperandclr_flags_helperrely on valid file handles and successfulfcntlcalls. It might be beneficial to add more descriptive error logs or ensure robust unit tests cover edge cases (e.g., invalid handles).
116-129: Replicate consistent logging or documentation for clr_flags.
Similarly toset_flags, the no-op approach forACE_NONBLOCKmay need explicit justification or documentation if blocking vs non-blocking is a known limitation on the simulator.
130-157: Double-check duplicate logic in clearing flags.
Implementations for clearing flags echo the approach for setting flags. Review whether to unify them into a single internal function or if separate logic is truly required.ACE/ace/config-integritySCA.h (2)
13-16: Document GHnet stack dependencies more thoroughly.
Since the file assumes GHnet-based network APIs are available, consider including references or conditions that alert maintainers if the GHnet libraries are absent or differ in version.
99-108: Avoid overlapping macros with config-integrity.h.
Some macros likeACE_LACKS_PIPEorACE_LACKS_SYSTEMappear in bothconfig-integrity.hand this file. Evaluate if you can consolidate them in one place to reduce maintenance overhead.ACE/ace/OS_NS_Thread.h (1)
264-285: Handle potential lock/unlock failures gracefully inLockGuard.
Currently, ifthread_mutex_lock/thread_mutex_unlockfails, there's no error handling. In a production environment, logging or asserting on errors might be helpful.ACE/ace/OS_NS_Thread.cpp (3)
1571-1577: Cond_timedwait not supported on INTEGRITY.
Marking it withACE_NOTSUP_RETURNis appropriate if native APIs do not provide suitable timed waiting. Consider adding explanatory logging or documentation, so callers can detect unavailable functionality more easily.
3323-3405: New ACE_Int178_Stack_Manager class.
This class effectively provides stack allocation for INTEGRITY-178B tasks. The design looks solid and uses an internal lock to manage the pool. Confirm that the maximum concurrency is adequately documented (NUM_DYNAMIC_THREADS) to avoid unexpected out-of-stacks scenarios.
3994-4059: Extended thr_create logic for INTEGRITY-178.
Allocating or using a pre-allocated stack, adjusting priority to match the parent, and storing thread arguments in a global map are valid workarounds for the OS’s API constraints. The overall flow seems correct.ACE/ace/OS_NS_Thread.inl (3)
54-57: Null-check consideration before accessing preallocated_object.
The code properly retrieves the preallocated object and referencesACE_INTEGRITY_TSS_Impl. Make sure thatACE_Object_Manager::preallocated_object[ACE_Object_Manager::ACE_INTEGRITY_TSS_IMPL]is guaranteed to be non-null prior to use. It is typically guaranteed by ACE’s object manager, but a null-check (or assertion) here could further safeguard against unexpected states.
1896-1897: Check for consistency in error handling on failure.
Releasing the semaphore via::ReleaseSemaphore(*s) == Success ? 0 : -1;is straightforward. In the event of failure, consider settingerrno(if the platform library sets or expects a specific error code) for alignment with other OS branches that seterrnoon failure.
1902-1903: Consider setting errno when trywait fails.
For::TryToObtainSemaphore(*s), consider mimicking the semantics of other branches (e.g., settingerrno = EBUSYif the attempt fails because the semaphore is already locked). This would maintain consistency with the rest of the codebase’s handling of “try” operations.ACE/ace/config-integrity-common.h (1)
131-153: Remove or consolidate macros if feasible.
WhenACE_NO_GHS_LIBIOis defined, many I/O macros are redefined as lacking. Confirm which macros are genuinely needed. Duplicate or stale macros can obscure code intent.ACE/ace/Single_Input_Reactor.cpp (2)
17-20: Clarify return type usage insize().
This returnsevent_handler_.handler() != nullptr;as asize_t. While functionally correct, it might cause confusion since asize_timplies the number of handlers, but this is effectively0or1. Consider making the intent clearer.- return event_handler_.handler () != nullptr; + return event_handler_.handler () ? 1 : 0;
37-45: Check for error returns inhandle_input().
Currently, any non-zero return is passed back up, but there is no specialized handling for errors or disconnections. Consider adding logging or a fallback action (e.g., removing the handler) whenhandle_input()fails.ACE/ace/Single_Input_Reactor.h (1)
117-119: Document or handle thenotify()no-op.
Since notifications are not supported, return an error code or update documentation to clarify behavior, preventing incorrect assumptions about asynchronous notifications.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (52)
.gitignore(2 hunks)ACE/NEWS(1 hunks)ACE/ace/ACE.inl(2 hunks)ACE/ace/Asynch_Pseudo_Task.cpp(1 hunks)ACE/ace/CDR_Size.h(2 hunks)ACE/ace/CDR_Size.inl(3 hunks)ACE/ace/CDR_Stream.cpp(3 hunks)ACE/ace/CDR_Stream.h(3 hunks)ACE/ace/CDR_Stream.inl(3 hunks)ACE/ace/Flag_Manip.cpp(2 hunks)ACE/ace/High_Res_Timer.cpp(2 hunks)ACE/ace/Malloc_Base.h(1 hunks)ACE/ace/Mem_Map.cpp(2 hunks)ACE/ace/OS_NS_Thread.cpp(20 hunks)ACE/ace/OS_NS_Thread.h(2 hunks)ACE/ace/OS_NS_Thread.inl(23 hunks)ACE/ace/OS_NS_arpa_inet.cpp(2 hunks)ACE/ace/OS_NS_arpa_inet.inl(1 hunks)ACE/ace/OS_NS_dirent.inl(3 hunks)ACE/ace/OS_NS_fcntl.cpp(1 hunks)ACE/ace/OS_NS_signal.inl(7 hunks)ACE/ace/OS_NS_stdio.h(1 hunks)ACE/ace/OS_NS_stdlib.cpp(2 hunks)ACE/ace/OS_NS_sys_select.inl(1 hunks)ACE/ace/OS_NS_sys_socket.inl(3 hunks)ACE/ace/OS_NS_sys_stat.inl(4 hunks)ACE/ace/OS_NS_sys_utsname.cpp(1 hunks)ACE/ace/OS_NS_time.h(11 hunks)ACE/ace/OS_NS_time.inl(2 hunks)ACE/ace/OS_NS_unistd.inl(11 hunks)ACE/ace/Object_Manager.cpp(1 hunks)ACE/ace/Object_Manager.h(2 hunks)ACE/ace/POSIX_Asynch_IO.h(1 hunks)ACE/ace/SOCK_Dgram_Mcast.inl(1 hunks)ACE/ace/Single_Input_Reactor.cpp(1 hunks)ACE/ace/Single_Input_Reactor.h(1 hunks)ACE/ace/Task.cpp(1 hunks)ACE/ace/ace.mpc(1 hunks)ACE/ace/ace_for_tao.mpc(1 hunks)ACE/ace/config-integrity-common.h(1 hunks)ACE/ace/config-integrity.h(1 hunks)ACE/ace/config-integrity178.h(1 hunks)ACE/ace/config-integritySCA.h(6 hunks)ACE/ace/config-macros.h(3 hunks)ACE/ace/os_include/os_fcntl.h(1 hunks)ACE/ace/os_include/os_signal.h(1 hunks)ACE/ace/os_include/os_stdio.h(2 hunks)ACE/ace/os_include/sys/os_types.h(1 hunks)ACE/tests/Dynamic_Priority_Test.cpp(1 hunks)TAO/docs/Options.html(1 hunks)TAO/tao/Strategies/advanced_resource.cpp(17 hunks)TAO/tao/Strategies/advanced_resource.h(1 hunks)
🧰 Additional context used
🧬 Code Definitions (8)
ACE/ace/Mem_Map.cpp (1)
ACE/ace/OS_NS_fcntl.cpp (4)
open(12-134)open(14-18)open(137-240)open(138-141)
TAO/tao/Strategies/advanced_resource.cpp (2)
TAO/tao/default_resource.cpp (6)
init(169-508)init(170-170)report_option_value_error(1146-1155)report_option_value_error(1147-1149)input_cdr_msgblock_allocator(865-883)input_cdr_msgblock_allocator(866-866)TAO/tao/Strategies/advanced_resource.h (6)
init(47-47)report_option_value_error(105-106)input_cdr_msgblock_allocator(91-91)report_unsupported_error(108-108)TAO_Advanced_Resource_Factory(35-130)TAO_Advanced_Resource_Factory(43-43)
ACE/ace/config-macros.h (1)
ACE/ace/OS_NS_Thread.cpp (2)
ACE_UNUSED_ARG(4547-4547)ACE_WIN32(3054-3063)
ACE/ace/CDR_Stream.h (1)
TAO/tao/CDR.h (16)
os(468-469)os(470-471)os(472-473)os(474-475)os(476-477)os(478-479)os(480-481)os(482-483)os(484-485)os(486-487)os(488-489)os(490-491)os(492-493)os(494-495)os(496-497)os(499-500)
ACE/ace/OS_NS_Thread.cpp (1)
ACE/ace/OS_NS_Thread.h (15)
ACE_OS_Recursive_Thread_Mutex_Guard(2079-2079)ACE_OS_Recursive_Thread_Mutex_Guard(2082-2082)ACE_OS_Recursive_Thread_Mutex_Guard(2098-2098)guard(314-314)guard(353-353)id(312-347)id(312-312)id(351-364)id(351-351)id(752-752)id(755-755)id(758-758)thr_id(1777-1777)thr_join(1677-1677)thr_join(1684-1686)
ACE/ace/OS_NS_Thread.h (3)
ACE/ace/OS_NS_Thread.cpp (6)
lock(2385-2392)lock(2385-2385)id(3377-3381)id(3377-3377)id(3385-3398)id(3385-3385)ACE/ace/Single_Input_Reactor.cpp (2)
lock(47-51)lock(47-47)ACE/ace/OS_NS_stdlib.cpp (2)
exit(43-63)exit(44-44)
ACE/ace/Single_Input_Reactor.cpp (1)
ACE/ace/Single_Input_Reactor.h (25)
initialized(135-135)size(136-136)register_handler(42-43)register_handler(44-46)register_handler(48-49)register_handler(51-54)register_handler(55-57)register_handler(58-62)register_handler(63-65)handle_events(34-34)handle_events(36-36)handler(129-131)handler(132-133)lock(137-137)open(15-20)close(30-30)remove_handler(67-68)remove_handler(69-70)remove_handler(71-72)remove_handler(73-76)remove_handler(77-77)notify(117-119)find_handler(127-127)restart(143-143)restart(144-144)
ACE/ace/Single_Input_Reactor.h (1)
ACE/ace/Single_Input_Reactor.cpp (12)
size(17-20)size(17-17)restart(304-307)restart(304-304)restart(309-312)restart(309-309)purge_pending_notifications(271-274)purge_pending_notifications(271-271)owner(294-297)owner(294-294)owner(299-302)owner(299-299)
🪛 Cppcheck (2.10-2)
TAO/tao/Strategies/advanced_resource.cpp
[error] 104-104: There is an unknown macro here somewhere. Configuration is required. If ACE_TEXT is a macro then please configure it.
(unknownMacro)
ACE/ace/Single_Input_Reactor.cpp
[error] 21-21: There is an unknown macro here somewhere. Configuration is required. If ACE_BEGIN_VERSIONED_NAMESPACE_DECL is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (129)
ACE/ace/Task.cpp (1)
272-272: Corrected terminology in commentThe terminology in the comment has been corrected from "case" to "cast", which is the accurate technical term when referring to the
static_castoperation in the following line.ACE/ace/OS_NS_time.inl (2)
258-258: Updated compiler detection macroThe change from
ghsto__ghs__uses the standard predefined macro for the Green Hills Software compiler, which improves compiler detection reliability.
281-281: Standardized compiler detection macroConsistent with the previous change, the Green Hills Software compiler detection has been updated from
ghsto__ghs__for better portability and compliance with standard compiler identification practices.ACE/ace/OS_NS_sys_select.inl (1)
37-37: Improved compiler warning suppressionAdded
ACE_UNUSED_ARG (timep)to suppress potential compiler warnings about an unused variable in theACE_LACKS_SELECTcode path. This is a good defensive programming practice.ACE/ace/OS_NS_stdio.h (1)
137-137:✅ Verification successful
Standardized INTEGRITY macro usage
Changed from
!defined (INTEGRITY)to!defined (ACE_INTEGRITY), which follows the ACE naming convention for platform-specific macros. This improves consistency and helps avoid potential macro name conflicts.
🏁 Script executed:
#!/bin/bash # Verify consistency in INTEGRITY macro usage across the codebase # Check for any remaining uses of bare INTEGRITY macros echo "Checking for remaining bare INTEGRITY macros that might need updating:" rg -n "defined\s*\(\s*INTEGRITY\s*\)" --type cpp echo "---" # Check for ACE_INTEGRITY usage for comparison echo "Current uses of standardized ACE_INTEGRITY macro:" rg -n "defined\s*\(\s*ACE_INTEGRITY\s*\)" --type cppLength of output: 5749
Verified standardized macro usage in ACE/ace/OS_NS_stdio.h
- The change from using
!defined(INTEGRITY)to!defined(ACE_INTEGRITY)is correctly applied in this file.- This update now adheres to the ACE naming convention, helping to avoid potential macro conflicts.
- Note: While bare
INTEGRITYreferences still appear in a couple of test and TAO files, these seem isolated to those modules and can be reviewed separately if needed.ACE/ace/OS_NS_sys_stat.inl (4)
22-25: Enhanced error handling for unsupported creat functionThe addition of
ACE_UNUSED_ARGmacros for parameters andACE_NOTSUP_RETURN (-1)improves error handling for platforms where the creat function isn't available. This prevents compiler warnings about unused parameters and clearly indicates the operation is unsupported.
68-70: Improved parameter handling for fstat function on unsupported platformsThe added
ACE_UNUSED_ARGmacros forhandleandstpparameters follow the same pattern as other functions in this file, properly marking parameters as unused when the function is not supported. This makes the code more consistent and prevents compiler warnings.
154-157: Enhanced error handling for mkdir function on unsupported platformsThe addition properly handles platforms where mkdir functionality is missing by marking parameters as unused and returning an appropriate error code. This is consistent with the error handling approach used throughout the ACE codebase.
203-204: Improved parameter handling for stat functionThe added
ACE_UNUSED_ARGmacros for thefileandstpparameters ensure proper handling of unused parameters on platforms where the stat function is not available. This makes the code more portable and avoids compiler warnings.ACE/ace/config-macros.h (5)
246-247: Updated Green Hills compiler identifierThe change from
ghsto__ghs__correctly uses the standard predefined macro for the Green Hills compiler. This makes the compiler detection more reliable.
257-261: Consistent use of Green Hills compiler identifierThe updated compiler identification macro consistently uses
__ghs__instead ofghsthroughout the conditional blocks, ensuring proper detection of the Green Hills compiler.
523-524: Updated documentation to reference INTEGRITY instead of pSoSThe comment has been updated to reference INTEGRITY instead of pSoS, which is in line with the PR's objective of updating support for Green Hills INTEGRITY and INTEGRITY-178 RTOS.
534-539: Added support for INTEGRITY platform thread function return typeNew conditional block properly handles the INTEGRITY platform's thread function behavior, setting a specific return type for INTEGRITY and INTEGRITY-178 Task API. The comments clearly explain the different behavior of entry point functions in this environment.
281-284: Updated version namespace handling for Green Hills compilerModified the conditional preprocessor directives to use the correct
__ghs__identifier for the Green Hills compiler, ensuring proper versioning behavior on this platform.ACE/ace/OS_NS_time.h (5)
51-52: Improved function signature formattingThe spacing in the
ace_timezone()function signature has been updated for better readability and consistency with the project's coding style.
128-136: Enhanced difftime function implementation readabilityThe formatting changes in the
ace_difftimefunction improve code readability while maintaining functionality. The spacing around preprocessor directives is now more consistent.
157-162: Improved enum declaration formattingThe
ACE_HRTimer_Openum declaration has been reformatted with each enumeration value on its own line, improving readability and making the code easier to maintain.
237-237: Modernized default parameter using nullptrChanged the default value for
remainingparameter from0tonullptr, which is the proper C++11 way to represent a null pointer. This improves type safety and readability.
267-267: Updated default parameter to use nullptrChanged the default value for
tlocparameter from0tonullptr, aligning with modern C++ practices. This improves type safety by using the proper null pointer literal.ACE/ace/ace.mpc (1)
219-219: Added support for Single Input ReactorThe new
Single_Input_Reactor.cppfile has been added to the project, which implements functionality for a single input reactor pattern. This aligns with the PR objective of enhancing support for INTEGRITY RTOS.Based on the PR objectives, this new component likely provides specific reactor implementation optimized for the INTEGRITY RTOS environment.
ACE/ace/ace_for_tao.mpc (1)
155-155: Addition of Single_Input_Reactor to ACE_FOR_TAO looks appropriate.The introduction of
Single_Input_Reactor.cppaligns with the updated support for Green Hills INTEGRITY. Since this PR aims to enhance INTEGRITY RTOS support, this component is likely needed for proper integration.ACE/ace/SOCK_Dgram_Mcast.inl (1)
14-14: Macro naming standardization from INTEGRITY to ACE_INTEGRITY.This change replaces
!defined (INTEGRITY)with!defined (ACE_INTEGRITY)which follows the standard ACE naming convention where platform-specific macros have theACE_prefix. This creates consistency with other ACE platform-specific macros.ACE/tests/Dynamic_Priority_Test.cpp (1)
50-52: Include string operations header for INTEGRITY platform.The condition is expanded to include the INTEGRITY platform, ensuring that string operations are properly supported on this RTOS. This is an essential part of the PR's objective to enhance INTEGRITY support.
.gitignore (2)
20-23: Added INTEGRITY-specific file patterns to .gitignore.These new entries (
.gpj,.int,.custom_build_rule,.empty_html_file) appear to be Green Hills INTEGRITY development artifacts that should be excluded from version control.
39-41: Added build directories to .gitignore.The addition of
.vs/,Static_Release/, andStatic_Debug/directories to .gitignore is appropriate as these are build artifacts that shouldn't be tracked in version control.ACE/ace/ACE.inl (1)
219-219: Support for Green Hills INTEGRITY added correctly.The preprocessor condition has been extended to include
__ghs__compiler identification, allowing ACE to properly use socket operations (ACE_OS::send/ACE_OS::recv) rather than file operations on the Green Hills INTEGRITY platform. This change aligns with the PR objectives of enhancing support for the INTEGRITY RTOS.TAO/tao/Strategies/advanced_resource.h (1)
67-68: New reactor type added for single input systems.A new enum value
TAO_REACTOR_SINGLE_INPUThas been added to support the newly implementedACE_Single_Input_Reactortype mentioned in the AI summary. This change extends the available reactor types for systems with limited I/O capabilities, which is particularly relevant for embedded environments like INTEGRITY RTOS.ACE/NEWS (1)
21-21: Documentation updated to reflect INTEGRITY RTOS support improvements.The NEWS file has been appropriately updated to document the enhanced support for Green Hills INTEGRITY and INTEGRITY-178 tuMP RTOS, which aligns with the PR title and objectives. This documentation change ensures users are aware of this important platform support update.
ACE/ace/POSIX_Asynch_IO.h (1)
35-35: Macro name standardization to ACE_INTEGRITYThis change updates the preprocessor directive from
INTEGRITYtoACE_INTEGRITYto follow the standard ACE convention of prefixing platform-specific macros withACE_.ACE/ace/os_include/os_fcntl.h (1)
102-103: Added support for ACE_INTEGRITY178B non-blocking operationsThis change extends platform-specific support by defining
ACE_NONBLOCKas1when compiling for INTEGRITY-178B RTOS, ensuring correct behavior for non-blocking operations on this platform.ACE/ace/Malloc_Base.h (1)
189-189: Enhanced exception handling for INTEGRITY platformsThis change allows exception handling for memory allocation failures even when running on INTEGRITY platforms, provided that the
_HAS_EXCEPTIONSmacro is defined. This provides better flexibility when INTEGRITY configurations support C++ exceptions.ACE/ace/os_include/sys/os_types.h (1)
66-66: Added ACE_INTEGRITY support for ACE_LOFF_T typeThis change ensures that the large file offset type
ACE_LOFF_Tis correctly defined aslong longwhen compiling for INTEGRITY platforms, similar to VxWorks and LynxOS.TAO/docs/Options.html (1)
1514-1519: Documentation updated with new reactor typeThe addition of the
single_inputreactor type expands the options available for systems without an I/O de-multiplex function likeselect. This is a valuable addition to the documentation and aligns with the implementation ofACE_Single_Input_Reactorin the codebase.ACE/ace/Asynch_Pseudo_Task.cpp (2)
52-52: Improved signal handling portabilityChanged to use
ACE_OS::sigemptyset()instead of direct system call, enhancing portability across platforms, particularly for INTEGRITY RTOS.
54-54: Improved signal handling portabilityChanged to use
ACE_OS::sigaddset()instead of direct system call, consistently using the ACE OS abstraction layer for signal operations.ACE/ace/CDR_Size.h (2)
82-84: Added support for std::string_viewThe new
write_string_viewmethod enhances theACE_SizeCDRclass by adding support for the C++17std::string_viewtype, which provides a more efficient way to handle string data without unnecessary copying.
237-240: Added stream operator for std::string_viewThis adds the corresponding
operator<<overload forstd::string_view, completing the implementation of string view support in the CDR size calculation functionality.ACE/ace/os_include/os_stdio.h (2)
46-46: Standardized INTEGRITY macro namingChanged the condition from
INTEGRITYtoACE_INTEGRITY, following the consistent pattern of prefixing OS-specific macros withACE_. This improves maintainability and reduces the risk of macro name conflicts.
56-56: Updated comment to match macro nameUpdated the closing comment to reflect the new macro name, maintaining consistency throughout the code.
ACE/ace/Object_Manager.cpp (1)
238-241: Support for INTEGRITY RTOS with TSS emulation looks good.The addition of preallocation for
ACE_INTEGRITY_TSS_Implprovides proper thread-specific storage support for INTEGRITY RTOS environments that use TSS emulation. The conditional compilation checks all necessary macros correctly.ACE/ace/OS_NS_dirent.inl (3)
23-26: Enhanced handling for platforms lacking closedir capability.This addition properly handles platforms that lack the
closedirfunction by marking the parameter as unused. The code structure follows the existing pattern in the file.
42-47: Enhanced handling for platforms lacking opendir capability.This addition correctly handles platforms that lack the
opendirfunction by marking the parameter as unused and returning a not-supported error code. This is consistent with how other unsupported functions are handled.
62-67: Enhanced handling for platforms lacking readdir capability.This addition properly handles platforms that lack the
readdirfunction by marking the parameter as unused and returning a not-supported error code. The implementation is consistent with the approach used foropendirand other functions.ACE/ace/OS_NS_arpa_inet.cpp (1)
105-123: INTEGRITY RTOS implementation of inet_ntoa has potential thread safety concerns.The implementation provides a good fallback for INTEGRITY RTOS platforms lacking
inet_ntoa. While using a mutex addresses concurrent access issues, be aware that returning a static buffer can still cause problems if multiple threads store the returned pointer and use it after subsequent calls to the function.Note that this non-thread-safe behavior is consistent with the original POSIX
inet_ntoadesign, which is also not thread-safe.Consider adding a function that accepts a caller-provided buffer (like
inet_ntop) for situations requiring true thread safety:int ACE_OS::inet_ntoa_r(const struct in_addr addr, char *buf, size_t buflen) { if (buflen < INET_ADDRSTRLEN) return -1; ACE_UINT32 ipaddr = ntohl(addr.s_addr); sprintf(buf, "%d.%d.%d.%d", ((ipaddr & 0xff000000) >> 24) & 0x000000ff, (ipaddr & 0x00ff0000) >> 16, (ipaddr & 0x0000ff00) >> 8, (ipaddr & 0x000000ff)); return 0; }ACE/ace/Mem_Map.cpp (2)
189-193: Standardized preprocessor macro name to ACE_INTEGRITY.Replacing
INTEGRITYwithACE_INTEGRITYimproves consistency with ACE's naming conventions for platform-specific macros. This change ensures the code follows ACE's style guidelines without changing functionality.
322-326: Standardized preprocessor macro name to ACE_INTEGRITY.This change maintains consistency with the earlier update and ensures that
ACE_INTEGRITYis used consistently throughout the codebase for platform-specific feature detection.ACE/ace/Object_Manager.h (2)
43-45: Conditional forward declaration for ACE_INTEGRITY TSS
These lines conditionally introduce a forward declaration ofACE_INTEGRITY_TSS_Impl. This approach looks correct for ensuring the class is seen only whenACE_INTEGRITY,ACE_HAS_TSS_EMULATION, and absence ofACE_HAS_THREAD_SPECIFIC_STORAGEare confirmed.
306-308:✅ Verification successful
Enum entry for ACE_INTEGRITY TSS implementation
AddingACE_INTEGRITY_TSS_IMPLtoPreallocated_Objectis consistent with the existing pattern of conditional enumerations. Please verify that downstream references toACE_INTEGRITY_TSS_IMPLare guarded by the same#ifchecks to avoid undefined references.
🏁 Script executed:
#!/bin/bash # Search for any usage of ACE_INTEGRITY_TSS_IMPL to confirm it's guarded by the same #if conditions rg --context 3 "ACE_INTEGRITY_TSS_IMPL"Length of output: 2507
Downstream Guard Conditions Verified for TSS Enum Entry
All usages of
ACE_INTEGRITY_TSS_IMPLappear to be sufficiently guarded:
- In ACE/ace/Object_Manager.h, the enum entry is added under
#if defined(ACE_INTEGRITY) && defined(ACE_HAS_TSS_EMULATION) && !defined(ACE_HAS_THREAD_SPECIFIC_STORAGE), which is consistent with other conditional enumerations.- In ACE/ace/OS_NS_Thread.cpp and ACE/ace/Object_Manager.cpp, references to
ACE_INTEGRITY_TSS_IMPLare enclosed within conditional blocks that check for TSS emulation and the absence of thread-specific storage.- In ACE/ace/OS_NS_Thread.inl, the usage is guarded by
#elif defined(ACE_INTEGRITY). Although this differs slightly from the full condition, it aligns with the intended build assumptions for Integrity platforms.No unguarded references are found, so the risk of undefined references is mitigated.
ACE/ace/High_Res_Timer.cpp (2)
176-176: Macro update for GHS compiler
Switching fromghsto__ghs__more reliably detects the Green Hills compiler. This prevents misidentification when targeting GHS.
230-230:✅ Verification successful
Comment consistency with the updated GHS macro
You updated the comment to reflect__ghs__. Ensure there are no stale references to the old macro in adjacent comments or code.
🏁 Script executed:
#!/bin/bash # Find residual references to 'ghs' that might be obsolete rg "ghs"Length of output: 20278
Review Comment Update: Consistent ghs Macro Usage in High_Res_Timer.cpp
The verification confirms that the code at line 230 in ACE/ace/High_Res_Timer.cpp now properly uses the updated
__ghs__macro. No stale or outdated references were detected in the adjacent code or comments.ACE/ace/os_include/os_signal.h (1)
157-157:❓ Verification inconclusive
Refined condition for ACE_INTEGRITY
ReplacingINTEGRITYwithACE_INTEGRITYaligns with the rest of the codebase macros. Confirm that all references to the old macroINTEGRITYare either removed or updated for consistency.
🏁 Script executed:
#!/bin/bash # Search for all references to 'INTEGRITY' outside of 'ACE_INTEGRITY' usage rg "INTEGRITY"Length of output: 14559
Action: Verify Consistent Use of ACE_INTEGRITY
The change in
ACE/ace/os_include/os_signal.hreplacing the plainINTEGRITYwithACE_INTEGRITYis correct and aligns with the updated naming convention. However, our search still returns multiple occurrences of the word "INTEGRITY" in various parts of the codebase (e.g., in TAO/orbsvcs references, ACE tests, and configuration files). Please ensure that these remaining instances are intentional—whether they refer to legacy or vendor-specific usage—or if they should also be updated for consistency.
- Confirm that files such as TAO/orbsvcs/orbsvcs/AV/global.h and ACE/tests/Dynamic_Priority_Test.cpp are not adversely impacted by this inconsistency.
- Review configuration files (like those in ACE/include/makeinclude and ACE/ace/config-integrity*.h) to determine if the plain
INTEGRITYusage is correct in context or if it needs renaming.ACE/ace/OS_NS_stdlib.cpp (2)
1199-1204: Custom definition of __progname under ACE_INTEGRITY
Introducing a mutable global__prognamewhenACE_INTEGRITYand!ACE_USES_GHS_ISIMPPCare defined helps accommodate the simulator environment. Ensure that multiple compiled units do not clash over symbol redefinition.
1223-1225:✅ Verification successful
Assigning __progname with const_cast for substring
Stripping const from the pointer is generally acceptable here, but confirm that no subsequent code mutates the memory of a string literal to avoid undefined behavior.
🏁 Script executed:
#!/bin/bash # Look for modifications to __progname in the codebase that might mutate its content ast-grep --pattern $'assignment_expression(left: (field_expression) @lhs)' --jsonLength of output: 256
No mutation of __progname found – const_cast usage confirmed as safe
The verification script did not reveal any subsequent modifications to __progname that would indicate mutation of the string literal. The current implementation inACE/ace/OS_NS_stdlib.cpp(lines 1223-1225) is acceptable.ACE/ace/OS_NS_sys_socket.inl (2)
138-140: Confirm usage ofACE_LACKS_CLOSE
This code usesACE_LACKS_CLOSEinstead ofACE_LACKS_CLOSESOCKET, which was previously questioned. Ensure this is the intended macro for handling a missingclose()rather than a missingclosesocket().
949-949: No functional changes in#endifdirective
This removal or modification of the comment does not affect functionality. Thanks for keeping the preprocessor directives consistent.ACE/ace/OS_NS_fcntl.cpp (1)
109-117: Check for consistentopenhandling
The newly added branch forACE_LACKS_OPENproperly returnsACE_INVALID_HANDLEwhenopenis unavailable. The subsequentACE_INTEGRITY178Bbranch also discards thesaargument. If these conditions can overlap, ensure there’s no unexpected conflict in build configurations. Otherwise, this looks fine.ACE/ace/OS_NS_unistd.inl (8)
87-87: Handle performance overhead withsysconf
Replacing a directgetpagesize()call withACE_OS::sysconf(_SC_PAGESIZE)is acceptable. The overhead is typically minor, but verify it’s not in a critical performance path.
113-116: Graceful fallback forchdir
Returning “not supported” whenACE_LACKS_CHDIRensures portability.
137-140: Graceful fallback forrmdir
Returning “not supported” is appropriate for platforms that lackrmdir.
375-376: Noftruncatesupport
Marking parameters as unused and returning “not supported” is consistent.
387-391: Nogetcwdsupport
Returning0whenACE_LACKS_GETCWDis declared follows standard practice for unavailable functions.
956-957: Consider reordering preprocessor checks
As noted before, you could moveACE_LACKS_SELECTchecks beforeACE_HAS_NONCONST_SELECT_TIMEVALto streamline this logic.
1039-1042: Partialsysconfsupport for ACE_INTEGRITY
Providing_SC_PAGESIZEfor INTEGRITY is useful. Returning “not supported” for other names is correct.
1183-1187: Handle missingwrite()gracefully
This ensures consistent error reporting whenACE_LACKS_WRITEis defined. The approach is straightforward.ACE/ace/CDR_Size.inl (2)
155-164: Implementation for std::string_view writing looks correctAll logic is consistent with existing
write_stringusage, ensuring that empty views pass anullptr. This alignment with prior patterns is good.
413-420: Operator<< overload for std::string_view is consistentAdequately calls
write_string_view()and leverages existinggood_bitchecks. No concerns.ACE/ace/CDR_Stream.inl (2)
371-380: New write_string_view method is consistent with existing patternsThe method properly uses
x.empty() ? 0 : x.data(). This ensures that an empty view is treated similarly to a null string. Good job.
1399-1406: Operator<< overload for std::string_view aligns with existing patternsThe new operator simply calls
write_string_view(x)and checksos.good_bit(), which is consistent with the rest of the code. No issues found.ACE/ace/OS_NS_signal.inl (9)
73-74: Conditionally blocksigactionon INTEGRITY with POSIX-lacking setups.This extension neatly ensures that
sigactionis properly marked as unsupported when compiling under INTEGRITY RTOS lacking POSIX support, preventing accidental usage.
89-93: Graceful fallback forsigaddseton unsupported platforms.Returning
-1withACE_NOTSUP_RETURNand marking arguments unused is consistent with the existing strategy for unsupported signal functions.
108-108: End of conditional block.This
#endifcorrectly closes theACE_LACKS_SIGADDSETbranch, no issues found.
114-118: Conditional treatment forsigdelseton unsupported setups.Similar to
sigaddset, the approach for platforms lacking this functionality is logical and standard.
133-133: End of conditional block.Closes the
sigdelsetblock without complication.
139-142: Unsupportedsigemptysethandling.Good usage of
ACE_NOTSUP_RETURNmaintains a uniform fallback for missing signal APIs.
158-161: Fallback forsigfillset.Ensures consistent behavior when
sigfillsetis not available, matching the existing code pattern.
177-181: Unsupportedsigismemberhandling.The approach to return
-1and unused arguments is coherent with the other not-supported sections.
202-202: Closing the final conditional.No issues; completes the compilation guard correctly.
ACE/ace/CDR_Stream.cpp (2)
1710-1721: Conditional try/catch block for string resizing.When exceptions are enabled (
!ACE_STD_ALLOCATOR_NOTHROW), the added try/catch block properly handlesstd::bad_allocby returningfalse. This is a robust measure against memory allocation failures.
1770-1781: Exception handling for wstring resizing.Again, selectively wrapping
std::wstring::resize()with try/catch is a sound approach to gracefully handle potential allocation failures.ACE/ace/config-integrity178.h (10)
1-4: Header guard and file structure.The file is well-guarded, preventing multiple inclusions. No immediate issues.
6-10: Descriptive comment block.The concise overview of INTEGRITY-178B version info is helpful for maintainers.
12-12: Includingconfig-integrity-common.h.Ensures reusability and consistent definitions across Integrity-based configs.
14-19: IntroducingACE_INTEGRITY178B.The note states that defining
ACE_INTEGRITY178Balso impliesACE_INTEGRITY. Verify thatACE_INTEGRITYis indeed automatically defined in a central place (e.g.,config-integrity-common.h), matching the documentation.
21-72: Excluding unavailable POSIX APIs.Comprehensive set of macros disabling unsupported functions, maintaining consistency with typical ACE patterns for RTOS porting.
74-105: Optional library disablement for GHNet-178.Conditionally disabling network utilities while retaining socket functions is sound for minimal embedded setups.
107-110: Marking certain headers as absent.Disabling
dlfcn.handregex.his standard if the OS libraries are unavailable.
111-115: C++17 limitations.Flags missing standard library features like
std::wstringorstd::string_view, which is important for controlling usage.
116-121: Emulating TSS for thread-specific data.Choosing ACE’s TSS emulation fits well when the underlying RTOS does not provide native TSS.
123-123: Header guard closure.Cleanly ends the file with no open macros left.
ACE/ace/CDR_Stream.h (2)
60-63: Conditional include for<string_view>looks good.
These lines correctly ensure<string_view>is only included whenACE_LACKS_STD_STRING_VIEWis undefined, preserving compatibility with older compilers.
1447-1450: Operator overload forstd::string_viewaligns well with existing string operators.
No issues noted; this maintains consistency with other overloads in the file and is properly guarded byACE_LACKS_STD_STRING_VIEW.ACE/ace/OS_NS_arpa_inet.inl (2)
13-50: Simple dotted-decimal parsing logic forinet_addris clear and correct.
The implementation validates each segment (0–255) and setsvalidto false if the segment is out of range or formatting is incorrect. This is a straightforward approach and should suffice for basic IPv4 parsing needs.
62-74: Conditionalinet_ntoasupport is properly handled.
Falling back toACE_NOTSUP_RETURN(0)ifACE_LACKS_INET_NTOAis defined, and otherwise calling the system function, cleanly integrates platform-specific behavior.TAO/tao/Strategies/advanced_resource.cpp (10)
28-28: Includingace/Single_Input_Reactor.his appropriate.
This addition aligns with the new reactor type support and ensures the symbol is visible for the single-input reactor case.
94-107: Disabling the advanced resource factory if already disabled.
The early return warns users that their Resource_Factory directives will be ignored if this factory is disabled. This preserves existing behavior and avoids confusion.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 104-104: There is an unknown macro here somewhere. Configuration is required. If ACE_TEXT is a macro then please configure it.
(unknownMacro)
128-133: Dropping support for-ORBReactorRegistryis clearly communicated.
Emitting an error here is an appropriate way to inform users that this feature is no longer available.
135-147: Obsolete-ORBReactorLockoption is managed gracefully.
Providing a debug message to use-ORBReactorTypeinstead keeps backward-compatibility while guiding users toward the correct modern approach.
148-191: Extended-ORBReactorTypeto includesingle_inputis well-structured.
Addingsingle_inputas a CLI option and defaulting to error for unsupported reactor types ensures discoverability and maintainability.
192-245: Allocator configuration switches are handled consistently.
Each block checks for the specified option (e.g.,nullvs.thread) and falls back to the default if unrecognized. This pattern is clear and maintainable.
246-255:-ORBReactorThreadQueueoption recognized.
Selecting LIFO or FIFO is straightforward, and unrecognized values trigger an error message. This consistency mirrors other CLI parameter handling in this class.
267-270: Warning when-ORBReactorThreadQueueis used with unsupported reactor types.
This debug message alerts users that only the TP reactor can leverage custom thread queues, preventing misconfiguration.
391-391: Initialization ofACE_Reactor_Impl *impltonullptr.
Using a clear initialization improves readability and consistency. No concerns here.
400-466: AddingTAO_REACTOR_SINGLE_INPUTin the switch statement.
Placing the new case alongside existing reactors maintains clarity and consistency. The single input reactor is properly instantiated usingACE_Single_Input_Reactor.ACE/ace/config-integrity.h (2)
12-16: Ensure clarity about the ORBEXPRESS macro usage.
DefiningORBEXPRESSfor header compatibility can be confusing if other parts of the code might actually rely on an ORBExpress environment. Please verify that there's no unintended collision with real ORBExpress usage or third-party references that also check for this macro.
67-71: Review conditional ACE_LACKS_FCNTL definition.
WhenACE_NO_GHS_LIBIOis defined,ACE_LACKS_FCNTLis also defined. Confirm that any file operations relying on fcntl are adequately handled or wrapped to avoid missing functionality.ACE/ace/Flag_Manip.cpp (1)
66-78: Validate the ACE_NONBLOCK no-op logic for simulator.
Returning 0 without modifying the handle forACE_NONBLOCKmay mask actual desired behavior. Confirm that ignoring non-blocking mode on the INTEGRITY simulator aligns with application requirements.ACE/ace/config-integritySCA.h (5)
3-4: Header guard naming looks good.
Renaming the guard toACE_CONFIG_INTEGRITYSCA_His consistent with the file name. No issues found here.
23-25: Confirm necessity of ACE_LACKS_PWD_FUNCTIONS.
If user/password functionality is unsupported, ensuring that references to these calls are guarded or stubbed is important. Otherwise, removing the macro if no longer needed can reduce confusion.
82-84: Verify suseconds_t and useconds_t unavailability.
Double-check thatACE_LACKS_SUSECONDS_TandACE_LACKS_USECONDS_Tare still correct on newer INTEGRITY versions. If these types are available, removing these macros might simplify the code.
131-132: Revisit ACE_LACKS_SWAB usage.
Make sureswab()is genuinely unavailable or undesirable to break on this platform. Any code referencingswab()should be addressed to avoid linking or runtime errors.
180-180: Finalize the end of header guard.
Everything looks consistent. Please confirm that no additional macros or includes are needed at the end of this config header before finalizing.ACE/ace/OS_NS_Thread.h (3)
227-245: Confirm definition ofTaskandSemaphore.
These typedefs rely onTaskandSemaphorebeing visible or forward-declared in this compilation unit or included headers. Please verify that both are properly declared for the INTEGRITY platform.
246-262: Macros appear consistent with ACE’s pattern.
Defining these flags as zero is typical for platforms that lack the corresponding thread features. No issues found.
287-370: Avoid forcing process termination when table is full.
CallingACE_OS::exitinget()is too aggressive for a library function. Consider returning an error, throwing an exception, or providing a fallback. A previous review similarly suggested removing this immediate exit for more graceful error handling.Apply a possible diff to remove the abrupt exit:
- if (first_slot == ACE_MAX_NUM_THREADS) - { - ACE_OS::exit (EXIT_FAILURE); - } + if (first_slot == ACE_MAX_NUM_THREADS) + { + // Return an error, throw, or handle gracefully instead of exiting + // e.g.: + // throw std::runtime_error("No available TSS slots"); + }ACE/ace/OS_NS_Thread.cpp (10)
24-27: Add include guard for<map>usage.
The inclusion of<map>here enables the usage of standard map-based data structures for INTEGRITY-specific functionality. This change appears correct and does not introduce noticeable issues.
1158-1162: Platform-specific condition handling block.
These compilation branches handle VxWorks, MQX, and INTEGRITY differently from other platforms for condition variables. The added#if/#else/#endiflines are consistent with the platform checks and appear logically sound.
1178-1178: Additional platform branch.
The new#elif defined (ACE_VXWORKS) || defined (ACE_MQX) || defined (ACE_INTEGRITY)clause ensures that paths for different RTOSes remain distinct. This helps integrate INTEGRITY alongside existing platforms.
1930-1937: Mutex emulation with a counting semaphore of initial value 1.
While using a semaphore of capacity 1 can act as a mutex, strictly ensure no logic requires recursive locking or ownership tracking. If multiple unlock calls can occur (or if ownership must be guaranteed), a more robust approach might be needed.Would you like to scan for any existing code paths that call
mutex_unlockmultiple times without ownership checks?
1979-1980: Consistent destruction logic for INTEGRITY.
Replacing the VXWorks branch with an INTEGRITY branch to callACE_OS::sema_destroy(m)cleanly matches the existing pattern and finalizes resource cleanup.
2073-2075: Use of WaitForSemaphore inside mutex_lock.
This block properly callsWaitForSemaphoreas a blocking mechanism. No issues are apparent, but confirm that semaphore creation always succeeds upstream to avoid potential mis-locking scenarios.
2263-2273: Non-blocking mutex attempt with TryToObtainSemaphore.
This logic cleanly parallels the VxWorks code. Returning-1witherrno = EBUSYon unavailable semaphores preserves the typical “trylock” semantics.
3305-3321: Global data structures for join semantics on INTEGRITY.
Storing semaphores in globalstd::mapobjects requires careful synchronization and lifecycle management. The addedACE_mutex_t integrity_task_args_lockis a good step; ensure all map accesses have matching locks.
3406-3441: Custom task entry adapter for INTEGRITY.
This function stores arguments in a global map, then retrieves them when the new task starts. The approach is reasonable given INTEGRITY-178’s API limitations. Ensure code thoroughly checks for missing keys in error scenarios (e.g., if the map entry was never set).
4223-4284:❓ Verification inconclusive
Custom thr_join for INTEGRITY using semaphores.
The approach to store a semaphore per thread ID, wait on it, and then remove it from the map upon completion is sound. Validate that semaphores are closed in all edge cases (e.g., forcibly terminated tasks) to avoid resource leaks.
🏁 Script executed:
#!/bin/bash # Description: Search the codebase for places where a task can be force-terminated (e.g. killTask). # Then verify that thr_join or thr_exit is invoked in all normal code paths. # Checking for Task termination calls fd . -t f -e cpp -e h -e c --exec rg "killTask|TerminateTask|AbortTask" {}Length of output: 78
I'll run an additional script to search for usage of
integrity_join_semasand verify if there are any references in the code handling forced termination or thread exit scenarios. This can help establish whether the semaphores are reliably cleaned up in all code paths.
🏁 Script executed:
#!/bin/bash # Search for any references to integrity_join_semas in the codebase rg "integrity_join_semas" .Length of output: 583
Review Verification:
The custom implementation ofthr_joinfor INTEGRITY using semaphores is implemented correctly, and the cleanup viaintegrity_join_semasinACE_OS::thr_joinappears consistent with the usage patterns observed in the codebase. However, the initial search for task forcible termination routines (e.g., via keywords likekillTask,TerminateTask, orAbortTask) produced no output, which makes it unclear if forced termination paths exist or if their corresponding cleanup routines are fully exercised. Please manually verify that any code paths associated with forced or abnormal termination (if present) trigger a proper cleanup—either by invokingthr_join/thr_exitor by explicitly releasing semaphores—to prevent resource leaks.
- Action Items:
- Double-check any termination routines or error paths that may bypass the normal thread join logic.
- Ensure that in all scenarios, the semaphore associated with a thread (
integrity_join_semas) is closed and erased, especially when tasks are terminated abnormally.ACE/ace/OS_NS_Thread.inl (2)
23-25: Include guard addition looks consistent.
These lines correctly conditionally includeace/Object_Manager.hfor the INTEGRITY environment when TSS emulation is enabled but native thread-specific storage is unavailable. The guard clauses are logically sound and align with ACE’s conditional include style.
1445-1452: New branch for sema_destroy on INTEGRITY.
Using::CloseSemaphore(*s)and returning 0 or -1 is consistent with the rest of the function in terms of error handling. The fallback toACE_NOTSUP_RETURN(-1)forACE_INTEGRITY178Balso follows the existing pattern. This looks correct for controlling the platform-specific support.ACE/ace/config-integrity-common.h (2)
56-56:✅ Verification successful
Confirm the necessity of
ACE_HAS_STRDUP_EMULATION.
Past discussions highlighted a desire to remove this macro if no longer needed. Validate that Integrity lacksstrdupand there is no standard alternative you could rely on.
🏁 Script executed:
#!/bin/bash # Searching for usage of ACE_HAS_STRDUP_EMULATION in codebase rg "ACE_HAS_STRDUP_EMULATION" -A 5Length of output: 6090
Verified: Retain
ACE_HAS_STRDUP_EMULATIONfor Integrity PlatformsAfter verifying the codebase:
- The macro is consistently defined in Integrity-specific configuration headers (e.g., in both
ACE/ace/config-integrity-common.handACE/ace/config-integritySCA.h) and is used within the string emulation implementations (as seen inOS_NS_string.inl,OS_NS_string.cpp, andOS_NS_string.h).- Change logs and the ACE documentation clarify that this emulation is necessary to control memory allocation behavior—particularly on platforms like Integrity where a native
strdupmay be absent or nonstandard.Please keep this macro definition to ensure consistent behavior on Integrity systems.
19-19:❓ Verification inconclusive
Consider verifying if
_REENTRANTis needed.
Modern compilers often default to thread-safe runtimes. Verify whether_REENTRANTis still required or if it can be removed to reduce potential confusion.
🏁 Script executed:
#!/bin/bash # Find references to _REENTRANT throughout the codebase. fd . -t f -e c -e cpp -e h -e hpp -x rg "_REENTRANT" {}Length of output: 1800
Action Required: Evaluate Usage of
_REENTRANTMacro
The repository search shows that_REENTRANTis defined in multiple configuration files—including inACE/ace/config-integrity-common.hat line 19—which suggests that its usage is intentional for ensuring compatibility (especially with legacy or nonstandard threading environments). Modern compilers might indeed default to thread-safe runtimes, but this macro may still be necessary for certain configurations. Please verify that your target platforms and toolchains no longer require an explicit definition of_REENTRANTbefore considering its removal. If testing and documentation confirm redundancy, updating or removing it could help reduce confusion.ACE/ace/Single_Input_Reactor.h (1)
42-44: Warn or assert unsupported reactor masks.
In prior discussions, it was noted that only read events are handled. Users might accidentally register write/exception masks. Adding an assertion or warning would reduce confusion.
| int ACE_Single_Input_Reactor::register_handler (ACE_Event_Handler *event_handler, ACE_Reactor_Mask) | ||
| { | ||
| event_handler_ = event_handler; | ||
| event_handler->add_reference (); | ||
| // disable ACE_NONBLOCK, see ACE_IPC_SAP::disable() | ||
| const auto handle = event_handler->get_handle (); | ||
| #if defined ACE_WIN32 || defined ACE_VXWORKS | ||
| u_long nonblock = 0; | ||
| ACE_OS::ioctl (handle, FIONBIO, &nonblock); | ||
| #else | ||
| ACE::clr_flags (handle, ACE_NONBLOCK); | ||
| #endif | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle return codes and release references.
event_handler->add_reference()increments the reference count, but there is no corresponding call to remove the reference inremove_handler()(not implemented here). This may lead to memory leaks.- The code disables non-blocking mode without checking the result of
ACE_OS::ioctl()orACE::clr_flags(). Logging or handling potential failures could be beneficial for debugging.
sonndinh
force-pushed
the
ACE-master-ghs
branch
from
e839e71 to
82d2e30
Compare
sonndinh
force-pushed
the
ACE-master-ghs
branch
from
82d2e30 to
845fe59
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ACE/ace/OS_NS_Thread.inl (1)
2267-2272: Return-1for sema_wait timeout on INTEGRITY.
Timeout is not currently supported, leading toACE_NOTSUP_RETURN(-1). If a timed wait is needed in the future, consider implementing the kernel-level timed call.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ACE/ace/OS_NS_Thread.inl(23 hunks)ACE/ace/OS_NS_fcntl.cpp(1 hunks)ACE/ace/OS_NS_sys_socket.inl(4 hunks)ACE/ace/OS_NS_sys_utsname.cpp(1 hunks)ACE/ace/config-mqx.h(0 hunks)
💤 Files with no reviewable changes (1)
- ACE/ace/config-mqx.h
🚧 Files skipped from review as they are similar to previous changes (3)
- ACE/ace/OS_NS_fcntl.cpp
- ACE/ace/OS_NS_sys_socket.inl
- ACE/ace/OS_NS_sys_utsname.cpp
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: ubuntu-24.04 clang++-15
- GitHub Check: ubuntu-24.04 g++-10 CORBA/e micro
- GitHub Check: ubuntu-22.04 clang++-13
- GitHub Check: ubuntu-22.04 g++-9
- GitHub Check: macos-14-C++
- GitHub Check: macos-13-C++
- GitHub Check: VS2022Release64
- GitHub Check: VS2019Debug32
- GitHub Check: VS2019Debug64Cxx20
- GitHub Check: VS2019Debug64Cxx17
- GitHub Check: VS2022WChar
- GitHub Check: VS2019WChar
- GitHub Check: VS2022Debug64
- GitHub Check: VS2019Release32
- GitHub Check: VS2019Debug64
- GitHub Check: VS2019Release64
- GitHub Check: MINGW64-gcc
- GitHub Check: alpine-3.18
- GitHub Check: ubuntu-24.04 g++
- GitHub Check: windows-2019 vs2019
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (15)
ACE/ace/OS_NS_Thread.inl (15)
23-26: Include the Object_Manager for Integrity TSS emulation.
These lines conditionally includeace/Object_Manager.hfor INTEGRITY TSS emulation. The approach looks consistent and does not introduce any obvious issues.
54-57: Provide INTEGRITY-specific TSS support intss_base().
This block fetches the preallocatedACE_INTEGRITY_TSS_Implobject and returns a reference to the TSS array. The logic appears correct and conforms to the TSS emulation interface.
1445-1452: Implementsema_destroysupport for INTEGRITY.
When not targeting INTEGRITY-178B,::CloseSemaphoreis used, otherwise it returns-1withACE_NOTSUP_RETURN. The fallback logic makes sense for now, though it leaves 178B without a destroy operation.
1751-1758: Initialize semaphore on INTEGRITY with unused parameters.
All additional parameters are unused, and the code simply calls::CreateSemaphore(count, s). This is acceptable for a system that does not require named or attribute-rich semaphores.
1895-1896: Implementsema_poston INTEGRITY usingReleaseSemaphore().
Straightforward mapping to the native API. No issues found.
2001-2003: Add non-blocking (trywait) semaphore call on INTEGRITY.
The code invokesTryToObtainSemaphore, aligning with the typical semantics of a try-wait operation.
2068-2069: Map blocking semaphore wait for INTEGRITY.
WaitForSemaphoreis the correct blocking call, returning 0 on success. Implementation looks consistent.
2388-2391: Return-1forsigwaiton INTEGRITY.
Signal waiting is marked as not supported. This is consistent with the rest of the file’s approach for unsupported OS calls.
2742-2745: UseExitTaskwithinthr_killfor INTEGRITY.
The code callsExitTaskand maps success/failure to 0/-1. This is reasonable for environment-specific thread termination.
2783-2786:thr_min_stacknot supported on INTEGRITY.
Currently returns0withACE_NOTSUP_RETURN(0). This is acceptable if the OS does not provide a minimum stack size API.
2849-2850:thr_self()on INTEGRITY returnsCurrentTask().
This integrates cleanly with existing patterns for retrieving a thread/task ID.
2885-2886: SetselftoCurrentTask()for thread handle retrieval.
Provides the appropriate handle for the current task on INTEGRITY.
3160-3161:thr_suspendon INTEGRITY mapped toHaltTask().
Suspending a task is equivalent to halting it in this environment. Implementation is logically consistent.
3253-3257: Initialize thread mutex for INTEGRITY.
Ignores additional parameters and callsACE_OS::mutex_init. This is fine if the OS does not support named or attribute-rich mutexes.
3301-3305: Wide-char variant ofthread_mutex_initon INTEGRITY.
Similarly, extra parameters are unused, with a direct call toACE_OS::mutex_init. No problems noted.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
ACE/ace/OS_NS_arpa_inet.inl (1)
56-68: Consider providing rationale for the conditional compilation structure.The conditional compilation has been restructured to exclude this implementation specifically when both
ACE_INTEGRITYandACE_LACKS_INET_NTOAare defined. This addresses jwillemsen's previous question by making the implementation available based onACE_LACKS_INET_NTOAregardless ofACE_INTEGRITY(except for the specific case where both are defined).It would be helpful to document why this specific combination requires different handling, perhaps with a brief comment explaining the rationale.
+// When both ACE_INTEGRITY and ACE_LACKS_INET_NTOA are defined, +// this implementation is provided in OS_NS_arpa_inet.cpp instead #if !(defined (ACE_INTEGRITY) && defined (ACE_LACKS_INET_NTOA))ACE/ace/OS_NS_Thread.cpp (2)
3326-3407: Stack management for INTEGRITY-178 is well implemented but has potential concurrency concerns.The
ACE_Int178_Stack_Managerclass provides a good solution for managing stacks for INTEGRITY-178 tasks. However, the thread safety of operations likereleaseneeds to be carefully considered, especially if tasks exit rapidly. If multiple threads attempt stack operations simultaneously, race conditions could occur.Consider adding more robust error handling for edge cases, such as when no stacks are available or when a stack is being used by another thread. Additionally, ensure the lock is properly used in all operations, including error handling paths.
3409-3446: Task adapter implementation is sound but consider edge cases.The task adapter retrieves stored arguments and calls the actual entry point. It also handles cleanup by removing entries from the global maps. The implementation works correctly for the normal case.
Consider adding more robust error handling for edge cases, such as when a thread argument isn't found or when semaphore operations fail. This would make the code more resilient.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
ACE/NEWS(1 hunks)ACE/ace/OS_NS_Thread.cpp(20 hunks)ACE/ace/OS_NS_Thread.inl(23 hunks)ACE/ace/OS_NS_arpa_inet.inl(1 hunks)ACE/ace/OS_NS_sys_socket.inl(3 hunks)ACE/ace/config-integrity-common.h(1 hunks)ACE/ace/config-integrity.h(1 hunks)ACE/ace/config-integrity178.h(1 hunks)ACE/ace/config-integritySCA.h(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- ACE/ace/OS_NS_sys_socket.inl
- ACE/ace/config-integrity.h
- ACE/ace/config-integrity178.h
- ACE/ace/config-integritySCA.h
- ACE/NEWS
🧰 Additional context used
🧬 Code Definitions (1)
ACE/ace/OS_NS_Thread.cpp (3)
ACE/ace/OS_NS_Thread.h (15)
ACE_OS_Recursive_Thread_Mutex_Guard(2079-2079)ACE_OS_Recursive_Thread_Mutex_Guard(2082-2082)ACE_OS_Recursive_Thread_Mutex_Guard(2098-2098)guard(314-314)guard(353-353)id(312-347)id(312-312)id(351-364)id(351-351)id(752-752)id(755-755)id(758-758)thr_id(1777-1777)thr_join(1677-1677)thr_join(1684-1686)ACE/ace/OS_NS_arpa_inet.cpp (1)
guard(113-113)ACE/ace/POSIX_Asynch_IO.h (3)
flags(933-933)flags(1100-1100)flags(1231-1231)
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: ubuntu-22.04 clang++-13
- GitHub Check: ubuntu-22.04 clang++-12
- GitHub Check: macos-14-C++
- GitHub Check: ubuntu-22.04 g++-9
- GitHub Check: VS2019Release64
- GitHub Check: VS2022Release64
- GitHub Check: ubuntu-22.04 g++-10
- GitHub Check: build
- GitHub Check: VS2022Debug64
- GitHub Check: VS2019WChar
- GitHub Check: VS2019Debug64
- GitHub Check: VS2019Debug64Cxx17
- GitHub Check: VS2019Release32
- GitHub Check: VS2019Debug64Cxx20
- GitHub Check: VS2019Debug32
- GitHub Check: macos-13-C++
- GitHub Check: VS2022WChar
- GitHub Check: MINGW64-gcc
- GitHub Check: windows-2019 vs2019
- GitHub Check: alpine-3.18
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (47)
ACE/ace/OS_NS_arpa_inet.inl (3)
13-48: Well-implemented custom inet_addr with proper validation.This implementation correctly handles string-to-IP conversion with appropriate validation:
- Parses each segment sequentially
- Properly validates numeric characters and dot separators
- Includes range checking (0-255) for each segment as previously suggested
- Correctly handles malformed inputs and returns INADDR_NONE when invalid
29-33: Range validation successfully implemented.The range validation for segments (0-255) has been correctly implemented as suggested in the previous review.
15-15:❓ Verification inconclusive
Use ACE_INET_Addr::is_ip_address to validate IP addresses.
The current implementation correctly validates IPv4 addresses, but ACE might already have a centralized validation function. Consider leveraging existing functionality for consistency if available.
🏁 Script executed:
#!/bin/bash # Check if ACE has a reusable IP validation function grep -r "is_ip_address\|validate_ip\|check_ip" --include="*.h" --include="*.cpp" --include="*.inl" .Length of output: 605
Action: Verify and possibly use the centralized IP validation method
I couldn’t find any direct reference to a function like
ACE_INET_Addr::is_ip_addressin the repository (the only related occurrence is in test code asis_ip_address_local). Please verify whether a centralized IP validation function exists in ACE. If it does, update the implementation inACE/ace/OS_NS_arpa_inet.inlto use that function for consistency. If not, ensure that the current inline IPv4 validation remains properly documented as the standard approach.
- Investigate ACE_INET_Addr (or similar) for an existing IP validation function.
- If available, refactor to call it in
ACE/ace/OS_NS_arpa_inet.inl.- Otherwise, document why the current implementation is maintained.
ACE/ace/OS_NS_Thread.cpp (13)
24-26: Good addition of the required include for INTEGRITY thread implementation.The addition of the
<map>include is necessary for the thread argument management system being implemented for INTEGRITY. The conditional inclusion ensures it's only brought in when needed.
208-212: Improved guard variable naming enhances code clarity.Naming the guard variables explicitly (instead of using unnamed temporaries) improves code readability and makes the RAII pattern more explicit. This change follows best practices for lock management.
Also applies to: 219-222, 260-263, 277-280
1158-1162: Appropriate extension of OS-specific condition handling to include INTEGRITY.Adding
ACE_INTEGRITYto the condition variable implementation allows INTEGRITY to use semaphores instead of events, consistent with howACE_VXWORKSandACE_MQXare handled.
1232-1236: Consistent inclusion of INTEGRITY in condition handling.These changes consistently apply the INTEGRITY condition handling pattern established above throughout the codebase.
Also applies to: 1262-1266
1571-1577: Good documentation of INTEGRITY-178 limitations for condition variables.The comment clearly explains that while regular INTEGRITY could support condition variable timeouts, INTEGRITY-178 lacks the necessary kernel calls. The code properly returns NOTSUP to indicate this limitation.
1929-1938: Implementation of mutex_init for INTEGRITY is correct but minimal.The implementation uses a counting semaphore with initial value of 1 to implement mutex functionality, which is a standard approach. The function properly handles arguments and returns appropriate success/failure values.
1974-1980: Appropriate mutex_destroy implementation for INTEGRITY.The implementation simply delegates to sema_destroy, which is appropriate given how mutex_init is implemented.
2074-2075: Implementation of mutex_lock for INTEGRITY is correct.The implementation uses WaitForSemaphore, which is the correct approach for the semaphore-based mutex implementation.
2262-2263: Implementation of mutex_trylock for INTEGRITY is correct.The implementation uses TryToObtainSemaphore, which is appropriate for a non-blocking mutex acquisition.
2336-2352: Potential race condition in mutex_unlock implementation for INTEGRITY.While the code attempts to prevent unlocking a mutex that isn't locked by checking the semaphore's value, there's a potential race condition between the calls to GetSemaphoreValue and ReleaseSemaphore. If multiple threads call this function simultaneously, the semaphore's value could exceed 1, breaking the mutex semantics.
3304-3324: Good workaround for INTEGRITY-178's lack of thread argument support.The implementation introduces a workaround for INTEGRITY-178's limitation where SetupTask doesn't support passing arguments to task entry points. Using a global map for thread arguments is a reasonable approach given the constraints.
4134-4154: Thread exit implementation for INTEGRITY is correct.The implementation properly releases resources and signals joining threads. The use of the LockGuard ensures thread safety when updating the global map.
4227-4290: Potential resource leak in thr_join semaphores.The thr_join implementation creates semaphores to coordinate joining threads. While the code removes the entry from the map (line 4272), on INTEGRITY-178 there doesn't appear to be a mechanism to destroy the semaphore object itself, which could lead to resource leaks over time.
Consider adding code to properly destroy the semaphore after the join operation completes, or document if the INTEGRITY OS automatically handles this cleanup.
ACE/ace/OS_NS_Thread.inl (20)
23-25: LGTM: TSS emulation support for INTEGRITYAdding the required include for Thread-Specific Storage emulation when ACE is being compiled for INTEGRITY.
54-57: Adding Thread Specific Storage support for INTEGRITYThis implementation provides thread-specific storage support for INTEGRITY by accessing the pre-allocated TSS emulation object. The code correctly uses the current task as the key to retrieve thread-specific storage.
1445-1449: Semaphore destruction implementation for INTEGRITYThe implementation correctly handles semaphore destruction for INTEGRITY by calling the platform's CloseSemaphore() function and properly checking the return value.
1749-1756: Semaphore initialization for INTEGRITYThis implementation adds support for creating semaphores in INTEGRITY by calling the platform's CreateSemaphore() function. The function correctly marks unused parameters and returns appropriate success/failure codes.
1893-1894: Added semaphore post operation for INTEGRITYThis code adds support for releasing a semaphore under INTEGRITY by calling the platform's ReleaseSemaphore() function and properly converting the return value to the ACE convention (0 for success, -1 for failure).
1999-2000: Added non-blocking semaphore wait for INTEGRITYThe implementation correctly adds support for trying to obtain a semaphore without blocking in INTEGRITY, using the platform's TryToObtainSemaphore() function.
2066-2067: Added blocking semaphore wait for INTEGRITYThis addition provides the implementation for waiting on a semaphore in INTEGRITY, using the platform's WaitForSemaphore() function.
2265-2270: Timed semaphore wait not supported for INTEGRITYThis implementation currently doesn't support timed semaphore waits for INTEGRITY, which is a limitation that may affect applications that rely on this functionality. The comment explains that while INTEGRITY does have kernel calls for timed semaphore waits, they're not being used to maintain consistency with INTEGRITY-178 behavior.
Is timed semaphore wait functionality critical for your application? If so, consider adding actual implementation using the INTEGRITY kernel calls in a future update.
2614-2614: Conditional exclusion of thr_join for INTEGRITY without POSIX threadsThis change correctly excludes the thr_join implementation for INTEGRITY when POSIX threads are not available, preventing compilation errors.
2731-2734: Thread termination implementation for INTEGRITYAdded support for terminating threads in INTEGRITY by calling ExitTask(). The implementation correctly converts between ACE and INTEGRITY error conventions.
2772-2775: Minimum stack size not supported for INTEGRITYThis implementation indicates that getting the minimum stack size for a thread is not supported for INTEGRITY. The comment explains why and mentions a related API that's available but doesn't provide the exact functionality needed.
Do you need minimum stack size functionality in your application? If so, consider implementing a workaround using GetTaskStackLimits() or defining a reasonable default minimum stack size.
2838-2839: Added thread self identification for INTEGRITYThis implementation correctly returns the current task ID for INTEGRITY using the platform's CurrentTask() function.
2874-2875: Thread handle retrieval for INTEGRITYThis implementation correctly sets the thread handle to the current task ID for INTEGRITY using the platform's CurrentTask() function.
2479-2480: Thread continuation not supported for INTEGRITYThis implementation indicates that continuing a suspended thread is not supported for INTEGRITY, which could be a limitation for applications that rely on this functionality.
Is thread continuation functionality needed in your application? If so, consider exploring if there's a different INTEGRITY API that could provide similar functionality.
3149-3150: Thread suspension implementation for INTEGRITYAdded support for suspending threads in INTEGRITY by calling HaltTask(). The implementation correctly converts between ACE and INTEGRITY error conventions.
3187-3188: Thread yield implementation for INTEGRITYThis implementation correctly adds support for yielding the CPU to other threads in INTEGRITY using the platform's Yield() function.
3242-3246: Thread mutex initialization for INTEGRITYThis implementation delegates to the generic mutex_init function for INTEGRITY, ignoring thread-specific parameters that aren't applicable. This approach is consistent with other platforms that don't have separate thread mutex types.
3288-3292: Thread mutex initialization with wide character name for INTEGRITYThis wchar_t version correctly mirrors the char* version, delegating to the generic mutex_init function for INTEGRITY.
3376-3376: Using generic mutex trylock for INTEGRITYThis change correctly includes INTEGRITY in the list of platforms that use the generic mutex_trylock implementation for thread_mutex_trylock, which is appropriate since INTEGRITY doesn't have separate thread mutex types.
2531-2537: Thread priority retrieval for INTEGRITYThis implementation correctly retrieves the active priority of a thread in INTEGRITY using GetActivePriority() and properly converts it to the expected integer format.
ACE/ace/config-integrity-common.h (11)
1-5: Well-structured header guard and platform identifierThe file begins with proper header guards and defines ACE_INTEGRITY macro which will be used throughout the codebase to identify the INTEGRITY platform.
6-18: Process management limitations for INTEGRITYThese definitions correctly identify the process management functions that are not available in INTEGRITY, which helps prevent the use of unsupported features.
19-23: Thread support configurationThese definitions correctly configure ACE for multi-threaded operation on INTEGRITY, defining _REENTRANT, ACE_MT_SAFE, and ACE_HAS_THREADS macros, along with a reasonable thread limit.
25-32: Signal handling configurationThese macros properly configure signal handling capabilities for INTEGRITY, indicating support for atomic signals and siginfo_t while noting the absence of certain headers and features.
34-40: POSIX compatibility settingsThis section correctly configures POSIX compatibility features, including conditional non-blocking I/O support and directory entry support, while also defining a standard page size.
42-78: Standard library feature deficienciesThis comprehensive set of macros identifies the numerous standard C/POSIX functions that are missing or have limitations in INTEGRITY, which is crucial for portable code.
80-89: System header and feature limitationsThis section correctly identifies system headers and features that are not available in INTEGRITY, with helpful comments about why some limitations exist.
90-104: I/O and platform-specific configurationsThese definitions handle I/O limitations and include the necessary INTEGRITY header, while also defining a sighandler type and specifying platform-specific optimizations.
105-116: Thread priority configurationThis section properly defines thread priority ranges and defaults for different scheduling policies, which is important for consistent thread behavior across platforms.
117-126: Additional feature configurationsThese macros define support for timed message blocks and lack of support for various UNIX features, along with exception handling configuration for the standard allocator.
128-151: Low-level I/O function configurationThis section conditionally defines macros to indicate missing low-level I/O functions when ACE_NO_GHS_LIBIO is defined, allowing ACE to adapt to different INTEGRITY configurations.
|
I will review another time later today |
| switch (flags) | ||
| { | ||
| case ACE_NONBLOCK: | ||
| ACE_UNUSED_ARG (handle); |
There was a problem hiding this comment.
Shouldn't this be a not sup return -1, the higher layer has explicitly requested nonblock which is not supported, currently this requested flag is just silently ignored
There was a problem hiding this comment.
This is specific to the INTEGRITY simulator which doesn't support non-blocking mode. Returning 0 allows the applications/tests to continue on the simulator and running with blocking mode is acceptable in those programs. That is, it isn't an error indication in this case.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
ACE/ace/OS_NS_Thread.cpp (3)
1572-1578: No cond_timedwait support
Here,ACE_NOTSUP_RETURN(-1)short-circuitscond_timedwaiton INTEGRITY/INTEGRITY-178. This is acceptable if the OS truly lacks timed waiting. However, be sure to document this limitation in the public API or porting notes so callers are not caught off guard by the “not supported” return.
3408-3408: Global static stack manager
ACE_Int178_Stack_Manager int178_stack_manageris created as a global static. Ensure that its construction and destruction order do not conflict with other global objects (especially in dynamic library scenarios) and confirm it is safe under concurrent access.
3997-4073: Ignoring user-supplied priority and reusing maximum parent priority
You override the passed-inprioritywith the parent’s maximum priority to comply with INTEGRITY-178’s requirements. If this is intentional, ensure it’s clearly documented for users who may expect their requested priority to be honored.ACE/ace/OS_NS_Thread.inl (3)
55-57: Consider null-checking the preallocated TSS object.When casting the raw pointer from
ACE_Object_Manager::preallocated_object, ensure the pointer is not null to avoid potential null dereferences.
2266-2271: No support for timed semaphore wait.Returning -1 with
ACE_NOTSUP_RETURNindicates clear intent, but you could consider implementing a partial timeout if INTEGRITY exposes any suitable APIs. Let me know if you need assistance.
2532-2537: Retrieve thread priority for ACE_INTEGRITY.The code correctly calls
GetActivePriority. Note that the scheduling policy remains unset; if you ever need to convey a policy, consider designating a default.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ACE/ace/OS_NS_Thread.cpp(20 hunks)ACE/ace/OS_NS_Thread.inl(24 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
ACE/ace/OS_NS_Thread.cpp (1)
ACE/ace/OS_NS_Thread.h (15)
ACE_OS_Recursive_Thread_Mutex_Guard(2079-2079)ACE_OS_Recursive_Thread_Mutex_Guard(2082-2082)ACE_OS_Recursive_Thread_Mutex_Guard(2098-2098)guard(314-314)guard(353-353)id(312-347)id(312-312)id(351-364)id(351-351)id(752-752)id(755-755)id(758-758)thr_id(1777-1777)thr_join(1677-1677)thr_join(1684-1686)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: macos-13-C++
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (20)
ACE/ace/OS_NS_Thread.cpp (8)
24-26: Include<map>for integrity tasks tracking
Usingstd::maphere makes sense for storing thread or task data keyed by an identifier. Just ensure your build environment supports this usage with the required C++ standard libraries, and verify that all concurrency controls (such as locks) are consistently applied when accessing the map.
1158-1162: Check consistency of added INTEGRITY condition
This addition to handleACE_INTEGRITYin thecond_broadcastlogic appears straightforward. Ensure that replicating the same code paths as VxWorks and MQX is correct for INTEGRITY, especially for signaling multiple waiting threads.
1182-1182: Verify uniform code paths for INTEGRITY
These added|| defined (ACE_INTEGRITY)conditions unify the behavior across VxWorks, MQX, and INTEGRITY for condition initialization and destruction. Confirm that INTEGRITY’s semaphore or condition handling truly matches the intended semantics.Also applies to: 1232-1232, 1262-1262
1930-1938: Using a counting semaphore as a mutex
You’ve implementedmutex_initunder INTEGRITY by creating a semaphore with an initial value of 1. This can work as a simple lock, but standard mutex behavior (e.g., robust error checking or recursion) may be absent. Ensure that all call sites accept basic counting-semaphore behavior.
1979-1981: Destroying a “mutex” with sema_destroy
Internally treating the mutex as a counting semaphore is consistent, but be aware this might surprise maintainers expecting traditional mutex semantics. Confirm that no advanced mutex features (like robust or recursive locks) are required.
3330-3332: Default 64 KB stack size for INTEGRITY-178
DefiningACE_INTEGRITY178B_DEFAULT_STACK_SIZEas 64 KB may be fine for smaller tasks, but verify that typical tasks won't exceed this, causing potential stack overflows.
3410-3419: Thread adapter logic for INTEGRITY
Storing the thread argument in a global map and immediately unlocking poses minimal risk of a race, since you start the task in a suspended state, then run it after insertion. Verify that no path can access the map key before insertion, and confirm that removing the entry upon exit truly handles all concurrency edge cases (e.g., multiple thr_joins).
4239-4301: Join logic for INTEGRITY
Yourthr_joinimplementation relies on a global map of semaphores keyed by thread handle. This works for a single join but does not explicitly handle multiple join calls on the same handle. Confirm that the code is never invoked more than once per thread, or add checks to avoid undefined behavior on subsequent joins.ACE/ace/OS_NS_Thread.inl (12)
23-25: Include directive looks correct for TSS emulation.No issues are apparent. Including
"ace/Object_Manager.h"under these conditions is consistent with the existing patterns for TSS handling.
1445-1449: Correct cleanup for INTEGRITY environments.Calling
::CloseSemaphore(*s)here properly finalizes the semaphore on INTEGRITY with a clear fallback on not supported scenarios.
1749-1756: Initialize semaphore for ACE_INTEGRITY.Passing all parameters as unused and calling
CreateSemaphoreis a straightforward approach. Looks consistent.
1894-1895: Implement sema_post using ReleaseSemaphore.This change correctly mirrors how other platforms post semaphores.
2000-2001: Implement trywait using TryToObtainSemaphore.The logic appropriately returns 0 on success and -1 otherwise, matching ACE’s existing patterns.
2066-2067: Implement blocking wait using WaitForSemaphore.This straightforward approach matches other semaphore-wait implementations across the code.
2732-2734: ExitTask usage for thr_kill on INTEGRITY.Using
::ExitTaskto terminate the target thread is consistent with the codebase’s approach for forcibly ending threads.
2773-2775: INTEGRITY thr_min_stack not supported.Returning 0 with
ACE_NOTSUP_RETURNis reasonable when no OS API for minimum stack size is available.
3149-3150: HaltTask called for thr_suspend on INTEGRITY.Confirm that
HaltTasktruly suspends (rather than prematurely terminating) the thread. If it’s a genuine suspend, this is fine.
3243-3247: Delegate thread_mutex_init to ACE_OS::mutex_init.Relying on the general mutex initialization for ACE_INTEGRITY is consistent and maintainable.
3289-3293: Wide-char version also delegates to mutex_init.Same reasoning as above; no inconsistencies found.
3377-3377: Trylock fallback for ACE_INTEGRITY.Using
ACE_OS::mutex_trylockaligns with how other platforms handle trylocks. No issues found.
| // Make sure the semaphore's value does not exceed its initial value, which is 1. | ||
| // This guards against incorrect unlocks from threads without holding the lock. | ||
| // This, however, does not prevent a race when two or more threads call this function | ||
| // simultaneously and the calls to GetSemaphoreValue and ReleaseSemaphore interleave. | ||
| // In that case, the semaphore's value could raise to above 1. | ||
| // So, only call unlock when know for sure the thread is holding the lock! | ||
| SignedValue curr_val; | ||
| if (::GetSemaphoreValue (*m, &curr_val) != Success) | ||
| return -1; | ||
|
|
||
| if (curr_val == 1) | ||
| { | ||
| errno = EPERM; | ||
| return -1; | ||
| } | ||
| return ::ReleaseSemaphore (*m) == Success ? 0 : -1; |
There was a problem hiding this comment.
Potential race in mutex_unlock
The code tries to prevent unlock when the semaphore value is already 1 to avoid exceeding the initial value. However, as you note in the comments, concurrent calls can interleave and increment the semaphore above 1. This breaks strict mutex logic.
Consider using a dedicated mutex API or adding an ownership/atomic check to reliably ensure no race in unlock operations.
Summary by CodeRabbit
New Features
Refactor / Maintenance
Chores