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
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (21)
🪧 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 |
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 |
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.
|
No other comments as far as I can tell, I assume you have done some local builds on linux/windows with running all tests to make sure it doesn't break anything (github CI only compiles the core and ACE unit tests but doesn't run any tests) |
I have a build on Windows. Is there a scoreboard that I can compare the status of the tests against? |
|
At this moment there is no public build |
I ran the ACE tests locally on a Windows build. There are 4 failed tests:
|
|
Adam started a Linux build and there is no new failed test compared to the TAO scoreboard's test results. Is there anything else to change? @jwillemsen |
|
Nothing to add at this moment, but I haven't reviewed all last changes in detail due to lack of time |
…by TAO - Calling ACE_OS::hostname within ACE_OS::uname causes infinite call loop
sonndinh
force-pushed
the
ACE-master-ghs
branch
from
03998d9 to
fa705dd
Compare
| @@ -214,13 +214,17 @@ ACE_OS::uname (ACE_utsname *name) | |||
|
|
|||
| return ACE_OS::hostname (name->nodename, maxnamelen); | |||
| #elif defined (ACE_INTEGRITY) | |||
| # if defined (ACE_LACKS_GETHOSTNAME) | |||
| ACE_UNUSED_ARG (name); | |||
There was a problem hiding this comment.
Why make this exception here, doesn't ACE_OS::hostname handle this?
There was a problem hiding this comment.
Calling ACE_OS::hostname causes an infinite calling loop between ACE_OS::hostname and ACE_OS::uname and gethostname is never called.
Summary by CodeRabbit
New Features
Refactor / Maintenance
Chores