Fix symbol leaks: replace internal MIOpen macros in driver#7254
Draft
BradPepersAMD wants to merge 7 commits intousers/brpepers/miopen_shared_utilsfrom
Draft
Fix symbol leaks: replace internal MIOpen macros in driver#7254BradPepersAMD wants to merge 7 commits intousers/brpepers/miopen_shared_utilsfrom
BradPepersAMD wants to merge 7 commits intousers/brpepers/miopen_shared_utilsfrom
Conversation
4 tasks
Phase 2 steps 1-4: Fix symbol leaks in common_utils and driver. Step 1: Make common_utils fully independent - Remove miopen/env.hpp dependency from random.hpp, replace with std::getenv() Step 2: Replace MIOPEN_THROW in driver (27 files) - Create common_utils/errors.hpp with COMMON_THROW macros using std::runtime_error (shared by driver, miopen_utils, and tests) - Replace all MIOPEN_THROW/MIOPEN_THROW_IF/MIOPEN_THROW_HIP_STATUS in driver with COMMON_THROW equivalents - Remove miopen::Exception catch from main.cpp - Replace miopen::StartsWith with std::string::starts_with (C++20) Step 3: Replace MIOPEN_LOG_CUSTOM in driver (driver-local) - Create driver/driver_log.hpp with DRIVER_LOG macros - Replace miopen::HIPErrorMessage with hipGetErrorString Step 4: Replace miopen/env.hpp in driver (driver-local) - Create driver/driver_env.hpp with std::getenv wrappers - Remove MIOPEN_DECLARE_ENV_VAR declarations Update all forwarding includes to use direct paths: - 19 driver includes: miopen/ forwarding → common_utils/ direct - 34 test includes: miopen/ forwarding → common_utils/ direct - 98 test includes: test/ forwarding → miopen_utils/ direct Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
The forwarding headers (e.g., src/include/miopen/rank.hpp) and their targets (e.g., common_utils/include/common_utils/rank.hpp) used the same include guard macro. This caused the target's content to be skipped when included through the forwarding header, since the guard was already defined by the forwarder. Fix: Remove include guards from all forwarding headers entirely. They contain no content of their own — just a single #include — so the target file's own guard provides all necessary protection. Affects 26 forwarding headers across src/include/miopen/, test/, and driver/. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
The forwarding headers in src/include/miopen/ include <common_utils/...> but the gtest build targets were not linking miopen_common_utils, so the include directory was not on the search path. This caused build failures for all gtest targets. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
Same fix as the previous gtest commit: the forwarding headers in src/include/miopen/ resolve to common_utils/ headers, so any target that includes MIOpen internals needs miopen_common_utils on its include path. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
The forwarding headers and removed driver/ cross-includes broke 8 test files in the unified miopen_gtest binary: - test/fusionHost.hpp: add back get_handle.hpp include that the miopen_utils version correctly omits but test code depends on - reduceextreme.hpp, reducecalculation.hpp: move miopen/miopen.h before kernel headers that static_assert on its macros - layout_transpose.cpp: add float16 typedef lost when the driver/conv_common.hpp cross-include was removed Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
Previously provided transitively by other headers; now needed explicitly after the driver cross-include cleanup. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
BradPepersAMD
force-pushed
the
users/brpepers/fix_symbol_leaks
branch
from
1864233 to
c567a50
Compare
…/fix_symbol_leaks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Building on #7253, replace internal MIOpen macro usage in the driver with shared or driver-local alternatives. Update all forwarding includes to use direct paths.
Base branch:
users/brpepers/miopen_shared_utils(PR #7253)Changes
Step 1: Make common_utils fully independent
miopen/env.hppdependency fromcommon_utils/random.hppstd::getenv()— common_utils now has zeromiopen/includesStep 2: Replace MIOPEN_THROW in driver (27 files)
common_utils/errors.hppwithCOMMON_THROW/COMMON_THROW_IF/COMMON_THROW_HIP_STATUSmacros usingstd::runtime_errormiopen::Exceptioncatch frommain.cppmiopen::StartsWithwithstd::string::starts_with(C++20)Step 3: Replace MIOPEN_LOG_CUSTOM in driver (driver-local)
driver/driver_log.hppwithDRIVER_LOG_ERROR/DRIVER_LOG_WARNING/DRIVER_LOG_INFO2miopen::HIPErrorMessagewithhipGetErrorStringStep 4: Replace miopen/env.hpp in driver (driver-local)
driver/driver_env.hppwithdriver_env::enabled()/driver_env::value_uint64()Update forwarding includes to direct paths
miopen/forwarding →common_utils/directmiopen/forwarding →common_utils/directtest/forwarding →miopen_utils/directRemaining internal deps in driver (59 includes, steps 5-10)
Test plan
grep -rn '#include.*miopen/' common_utils/returns nothinggrep -rn 'MIOPEN_THROW' driver/returns nothing🤖 Generated with Claude Code