Dyninst 13 - #12
Draft
dgaliffiAMD wants to merge 518 commits into
Draft
Conversation
clang complains: 'constexpr' non-static member function will not be implicitly 'const' in C++14; add 'const' to avoid a change in behavior [-Werror,-Wconstexpr-not-const]
- when compiling with clang 15 and 16 in addition to clang 14, allow a 40000 stack frame size
* Move elfutils logic into build_elfutils.sh * Whitespace * Copy in dependencies.versions This is no longer done in the base container because it didn't make sense there as that image should never change.
Dyninst now requires TBB >= 2019.9.
* Weekly build across all compiler versions and build types * Use DYNINST_WARNINGS_AS_ERRORS=ON instead of -Werror
Github only allows one CPU core per job, but any number of threads. Testing shows that N=2 threads reduces build time by 2-2.5x, N=3 by 2.2x, and N=4 increases build time.
This makes it consistent with std::condition_variable. Found using cppcheck: common/src/dthread.h:114:4: warning: Class 'CondVar' does not have a copy constructor which is recommended since it has dynamic memory/resource allocation(s). [noCopyConstructor] mutex = new mutex_t; ^ common/src/dthread.h:114:4: warning: Class 'CondVar' does not have a operator= which is recommended since it has dynamic memory/resource allocation(s). [noOperatorEq] mutex = new mutex_t;
…eak (dyninst#1427) * AddressTranslateSysV::adjustForAddrSpaceWrap: Fix C file descriptor leak Found using cppcheck: common/src/addrtranslate-sysv.C:1046:58: error: Resource leak: fd [resourceLeak] if (read(fd, &e_hdr, sizeof(e_hdr)) != sizeof(e_hdr)) return base; ^ common/src/addrtranslate-sysv.C:1048:28: error: Resource leak: fd [resourceLeak] if (e_hdr.e_phoff == 0) return base;
* AddressTranslateWin
Found using cppcheck:
common/src/addrtranslate-win.C:49:17: style: Virtual function 'init' is called from constructor 'AddressTranslateWin(PID pid,PROC_HANDLE phandle)' at line 168. Dynamic binding is not used. [virtualCallInConstructor]
virtual bool init();
* NodeIteratorPredicateObj
Found using cppcheck:
common/src/NodeIterator.h:293:18: style: Virtual function 'inc' is
called from constructor
'NodeIteratorPredicateObj(Graph::NodePredicate::Ptr
p,NodeIterator&b,NodeIterator&e)' at line 331. Dynamic binding is not
used. [virtualCallInConstructor]
* NodeIteratorPredicateFunc
Found using cppcheck:
common/src/NodeIterator.h:352:18: style: Virtual function 'inc' is
called from constructor
'NodeIteratorPredicateFunc(Graph::NodePredicateFunc
p,void*u,NodeIterator&b,NodeIterator&e)' at line 394. Dynamic binding is
not used. [virtualCallInConstructor]
* InstructionDecoder_x86
Found using cppcheck:
instructionAPI/src/InstructionDecoder-x86.h:74:49: style: Virtual function 'setMode' is called from constructor 'InstructionDecoder_x86(Architecture a)' at line 144. Dynamic binding is not used. [virtualCallInConstructor]
INSTRUCTION_EXPORT virtual void setMode(bool is64);
^
instructionAPI/src/InstructionDecoder-x86.C:144:28: note: Calling setMode
if(a == Arch_x86_64) setMode(true);
^
instructionAPI/src/InstructionDecoder-x86.h:74:49: note: setMode is a virtual function
INSTRUCTION_EXPORT virtual void setMode(bool is64);
* CFGFactor::destroy_block
Found using cppcheck:
parseAPI/h/CFGFactory.h:117:18: style: Virtual function 'free_block' is called from destructor '~CFGFactory()' at line 92. Dynamic binding is not used. [virtualCallInConstructor]
virtual void free_block(Block * b);
^
parseAPI/src/CFGFactory.C:92:5: note: Calling destroy_block
destroy_block(b);
^
parseAPI/src/CFGFactory.C:182:5: note: Calling free_block
free_block(b);
^
parseAPI/h/CFGFactory.h:117:18: note: free_block is a virtual function
virtual void free_block(Block * b);
* CFGFactor::free_func
Found using cppcheck:
parseAPI/h/CFGFactory.h:116:18: style: Virtual function 'free_func' is called from destructor '~CFGFactory()' at line 95. Dynamic binding is not used. [virtualCallInConstructor]
virtual void free_func(Function * f);
^
parseAPI/src/CFGFactory.C:95:5: note: Calling destroy_func
destroy_func(f);
^
parseAPI/src/CFGFactory.C:172:4: note: Calling free_func
free_func(f);
^
parseAPI/h/CFGFactory.h:116:18: note: free_func is a virtual function
virtual void free_func(Function * f);
* CFGFactor::free_edge
* ~freebsd_process
Found using cppcheck:
proccontrol/src/freebsd.C:1046:17: warning: Member variable 'freebsd_thread::is_exited' is not initialized in the constructor. [uninitMemberVar]
freebsd_thread::freebsd_thread(int_process *p, Dyninst::THR_ID t, Dyninst::LWP l)
^
proccontrol/src/freebsd.h:122:17: style: Virtual function 'getEventQueue' is called from destructor '~freebsd_process()' at line 795. Dynamic binding is not used. [virtualCallInConstructor]
virtual int getEventQueue();
^
proccontrol/src/freebsd.C:795:22: note: Calling getEventQueue
int eventQueue = getEventQueue();
^
proccontrol/src/freebsd.h:122:17: note: getEventQueue is a virtual function
virtual int getEventQueue();
* Mismatched printf format arguments These were found using cppcheck's invalidPrintfArgType_*. * Fix uninitialized member variables
* Protect against self-assignment in copy constructors
These were found by cppcheck:
common/src/Node.C:198:29: warning: 'operator=' should check for assignment to self to avoid problems with dynamic memory. [operatorEqToSelf]
NodeIterator &NodeIterator::operator=(const NodeIterator &rhs) {
^
common/src/Edge.C:108:29: warning: 'operator=' should check for assignment to self to avoid problems with dynamic memory. [operatorEqToSelf]
EdgeIterator &EdgeIterator::operator=(const EdgeIterator &rhs) {
These were found using cppcheck's nullPointerRedundantCheck.
These were detected by cppcheck's uninitMemberVar*.
These were found using cppcheck's uninitvar and eraseDereference. For the usages of iterator-like classes in process.C, they aren't strictly uninitialized variable usages since the classes in question are standard layout types. It would be better to replace the usages there with list construction to avoid the static check altogether.
* Shifting signed 32-bit value by 31 bits is undefined behavior This was found using cppcheck's shiftTooManyBitsSigned. * Signed to unsigned conversion in calculation These were found using cppcheck's signConversion. * Fix signed overflow This was found using cppcheck's integerOverflowCond. * Shifting negative value This was found using cppcheck's shiftNegativeLHS. * BPatch_addressSpace::deleteSnippet: fix enumeral conversion in conditional
* Local var leak in Symtab::addSymbol This was found using cppcheck's memleak. * memCache::doOperation Not technically a leak, but cppcheck can't see through the 'push_back'. * PCProcess::hasPassedMain Found using cppcheck's danglingTemporaryLifetime. * parse_func::calcParentFunc Found using cppcheck's danglingTemporaryLifetime. * int_iRPC::setBinarySize Found using cppcheck's publicAllocationError.
The following instruction decoded with the wrong length if the modrm operand specified a memory access as all the operand was incorrectly specified to be a register only operand: - vcvtpd2udq - vcvtss2usi - vcvttpd2qq - vcvtudq2pd - vcvtudq2ps - vpblendd - vpermpd
- update dyninst header files to directly include the standard header file defining symbols from the standard C++ library that are used by the dyninst header file; in some instances, the code relied on symbols being defined via an unrelated include file indirectly including the necessary header file leading to fragile code - minor other cleanups: remove unnecessary header files, remove definitions of names that are defined in a standard header file
There are several reasons for this. 1. It doesn't copy every member of the class 2. IBSTree doesn't have a copy constructor 3. mod_lookup_ and func_lookup_ are not copied, but are recreated in other member functions (e.g., mod_lookup()). This completely breaks the semantics of a copy ctor. 4. Resets _ref_cnt to 1
The definition was commented out in 3709ead in 2009.
It is never used.
…t#1450) Because there is a user-defined destructor, the compiler will not generate the special member functions (e.g., copy assignment operator). However, we explicitly delete them here to signal that this class is not copyable or movable. The destructor is also moved to the top of the class to be next to the other special member functions.
* Use default member initializers instead of initializer lists There are members missing from the lists. This also simplifies the constructors considerably. * Remove unhelpful message in default ctor * Delegate to default ctor in Symtab(MappedFile*) This fixes the bug of inconsistent base initialization and ensures the constructors are consistent. * Use delegating ctor for Symtab::Symtab(unsigned char*...) This fixes the following bugs 1. Invoke init_debug_symtabAPI before calling create_printf. 2. Because this constructor can return early due to errors, it's imperative that the object is completely constructed by some means _before_ such a return happens. Otherwise, references to this object and its destruction would have class invariants inconsistent with objects created by the other constructors. * Use delegating ctor for Symtab::Symtab(std::string...) Because this constructor can return early due to errors, it's imperative that the object is completely constructed by some means _before_ such a return happens. Otherwise, references to this object and its destruction would have class invariants inconsistent with objects created by the other constructors.
…ninst#1452) This fix does not address the thread safety issues in Symtab::parseFunctionRanges. That function is still thread unsafe and will require separate modifications.
* Convert mod_lookup_ to a value type This makes its existence a class invariant and makes Symtab::mod_lookup threadsafe. * Fix includes in Function.h so that it can be included in Symtab.h * Fix includes in Variable.h so that it can be included in Symtab.h * Use Function.h instead of forward declarations This removes duplicate typedefs, as well. * Convert mod_lookup_ to a value type This makes its existence a class invariant and makes its use in Symtab::parseFunctionRanges threadsafe.
According to the llvm page https://llvm.org/docs/AMDGPUUsage.html#relocation-records AMDGPU backend generates Elf64_Rela records. This fix added the case for EM_AMDGPU to static Region::RegionType getRelTypeByElfMachine such that it uses RELA instead of the default REL type.
It's usage was removed by 4be991a in 2021.
* Fix spelling of 'vpperm' stringification * Fix spelling of 'vpcomd' stringification * Fix spelling of 'vprolvd' stringification * Fix spelling of 'vprolvq' stringification * Remove duplicate entry for 'e_vpsrlvq' * Rename 'vpclmullqlqdq' to 'vpclmulqdq' * Rename 'punpcklqd' to 'punpckldq' * Replace 'punpckhqd' with 'punpckhqdq' * Replace 'pcmpgdt' with 'pcmpgtd' * Remove instruction 'vpcmpequd' This mnemonic does not exist in any of Intel Architecture Instruction Set Extensions, AMD Architecture Programmers Manual, or Intel Architecutre Software Developer's Manual. The encoding listed under 'SSET1E_66' is a reserved space. * Rename mnemonic for 'prefect_w' to 'prefetchw' * Rename 'pclmullqlqdq' to 'pclmulqdq' This is a pseudo-opcode for pclmulqdq when imm8 is 0x0. The encoding is 660F3A44, so the lookup table location is correct. * Remove 'mmxud' It is an unknown instruction and has no decode table entry. * Remove 'movslq' This is an unknown instruction and has no decode table entry. * Remove 'pcommit' This is an unknown instruction and has no decode table entry. * Rename 'cmpsd_sse' to 'cmpsd' * Fix spelling of 'vphaddsw' mnemonic * Fix spelling of 'vblendpd' mnemonic * Remove redundant copy of 'prefetchwt1' * Remove redundant 'phsubsw' * Remove redundant 'sarx' * Remove redundant 'shlx' * Remove redundant 'vblendvpd' * Remove redundant 'vcmppd' * Remove redundant 'vcmpps' * Remove redundant 'vcmpsd' * Remove redundant 'vcmpss' * Remove redundant 'vpmaddubsw' * Remove redundant 'vpmaddwd' * Remove redundant 'vpmovsdb' * Remove redundant 'vpmovsdw' * Remove redundant 'vpmovsqb' * Remove redundant 'vpmovsqd' * Remove redundant 'vpmovsqw' * Remove redundant 'vpmovswb'
…inst#1684) * Refactor IA_IAPI::isSyscall,isInterrupt into arch-specific files This also requires those members be defined for all classes derived from IA_IAPI.
* Treat x86 software interrupts as system calls * Correct detection of Linux vsyscall for 32-bit code The existing code did not work because the Operand formatter uses AT&T syntax and doesn't convert hex to decimal, so the '== "16"' check always failed. The convoluted logic in the visitor is needed because Dyninst does not generate AST for segment registers. Yet, it _does_ record if they are read/written. The AST for the 'gs' register was made a file scope static because its initialization would cause some glibc's to throw a __gnu_cxx::recursive_init_error when using multiple threads. * aarch64 - add svc Supervisor Call * ppc - add system call * Add int1, into to IA_x86::isInterrupt
Currently, all symbols are hidden by default and then explicitly made visible. This is the behavior we want for publishing Dyninst libraries, but unit testing the internal functionality requires access to these symbols. This option allows that.
This was broken by e7db16f in 2010.
- update COPYRIGHT files's notice to include 2024 - update copyright header in source files to reference COPYRIGHT
…yninst#1691) 165f19c and 091929d changed the parsing semantics of system calls such that software interrupts (e.g., `int` on x86) are always interpreted as system calls. The `hasCFT()` check on line 1824 now covers all of the system call cases because it considers those instructions to have a control flow target (CFT). Its non-use was verified against /usr/libx32/libc.so.6, /usr/lib/i386-linux-gnu/libc.so.6, /usr/lib/x86_64-linux-gnu/libc.so.6, and /usr/lib32/libc.so.6 on Ubuntu 22.04.
* Move isSyscall to instructionAPI * Use InstructionAPI::isSystemCall in LivenessAnalyzer::calcRWSets * Use InstructionAPI::isSystemCall in PCSensitiveTransformer::process * Use InstructionAPI::isSystemCall in Parse::parse_frame_one_iteration * Use InstructionAPI::isSystemCall in IA_IAPI::getNewEdges * Remove IA_IAPI::isSyscall It is replaced by InstructionAPI::isSystemCall.
* Move isInterrupt to InstructionAPI * Use InstructionAPI::isInterrupt in IA_IAPI::isInterruptOrSyscall * Use InstructionAPI::isInterrupt in LivenessAnalyzer::calcRWSets * Remove IA_IAPI::isInterrupt It is replaced by InstructionAPI::isInterrupt
- update dyninst version - update CHANGELOG.md - produce manuals with updated release dates and versions - fix .tex files to eliminate latex errors: - invalid char in symtabAPI/doc/API/Types/Type.tex - invalid char in parseAPI/doc/API/Function.tex - invalid char in patchAPI/doc/section/4_api_public.tex - invlide line break in parseAPI/doc/API/CodeObject.tex - restore parseAPI example code needed by docs removed in 6c2e31c Co-authored-by: James Kupsch <kupsch@vmbp15.local>
nullptr check in handleCondDirExits for dyninst relocation instrumenter
This can be used to ensure that all threads in a process are stopped before the breakpoint is executed.
…inst#2131) * Use a synchronized breakpoint for 'main' If a thread is spawned before reaching main (e.g., in a .ctor entry), proccontrol does not observe events from it and so assumes it is safe to remove the breakpoint at 'main'. It is unclear at this time why no events are observed. Particularly becuase there is a check that _should_ catch this case: BPatch::processCreate V BPatch_process::BPatch_process V PCProcess::createProcess V Process::createProcess V int_process::createProcess V PCProcess::bootstrapProcess() V assert( pcProc_->allThreadsStopped() ); co-authored by Kian Cossettini @ AMD * Ignore events for destroyed threads launched before main If a thread is launched and destroyed before main is reached, there could be events in the queue that belong to that dead thread. We should ignore them. This seems at odds with proccontrol's failure to detect that there are threads alive before main is reached, but there is a user-reported crash that is fixed by these changes. Co-authored-by: Kian Cossettini <Kian.Cossettini@amd.com>
## Motivation <!-- Explain the purpose of this PR and the goals it aims to achieve. --> - Fix to allow building Dyninst usint both bundled dependencies, from the parent project, and the installed system dependencies. - Required for ROCm/rocm-systems#4206. ## Technical Details <!-- Explain the changes along with any relevant GitHub links. --> - Revert commit e0d5940, which caused us unable to build with the system's dependencies. - If `Dyninst::Boost`, `Dyninst::TBB`, `Dyninst::ElfUtils`, or `Dyninst::LibIberty` exist, skip find_package() - This allows the parent project, like `rocprofiler-systems`, to provide bundled dependencies via Dyninst::* targets - Essential for bundled dependency support (ROCPROFSYS_BUILD_*=YES) ## JIRA ID - AIPROFSYST-354 ## Test Plan <!-- Explain any relevant testing done to verify this PR. --> Workflows tested in ROCm/rocm-systems#4206 ## Test Result <!-- Briefly summarize test outcomes. --> ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests. --------- Co-authored-by: Claude (Claude-Sonnet-4.5) <noreply@anthropic.com>
Fixes my issue: dyninst#2203. Cherry-picks dyninst@aab9cbd. Part of ROCM-21219 Co-authored-by: kupsch <kupsch@cs.wisc.edu>
Never c/c++ header versions provided by the gcc 15
requires that stdint.h and cstdint needs to be included excplicitly
to avoid build errors related to types like uint32_t.
Fixes following type of errors with gcc 15:
/therock/rocm-systems/projects/rocprofiler-systems/external/dyninst/common/src/sha1.C:112:5:
error: ‘uint32_t’ does not name a type
10.6 112 | uint32_t state[5];
and
‘INT32_MAX’ was not declared in this scope
Signed-off-by: Mika Laitio <mika.laitio@amd.com>
## Motivation <!-- Explain the purpose of this PR and the goals it aims to achieve. --> Cherry pick 635cacd from https://github.com/dyninst/dyninst This adds the `isSharedLib()` to `BPatch_object`, allowing a quicker way of filtering out shared libraries. This way we can avoid going through the SymTab API or `BPatch_module` ## Technical Details <!-- Explain the changes along with any relevant GitHub links. --> See dyninst#2215 ## Test Plan <!-- Explain any relevant testing done to verify this PR. --> ## Test Result <!-- Briefly summarize test outcomes. --> ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.
## Motivation Remove Boost dependency from Dyninst and use std and dyncompat as replacement. The removal of Boost dependency needs to be done for the reasons as described in the now closed PR on dyninst master branch: dyninst#2172 ## Technical Details Boost was dropped as an external dependency; Dyninst now builds against an internal dyncompat layer and C++17 standard library equivalents. ## Test Plan Tested with rocm-systems PR: (ROCm/rocm-systems#5406). ## Test Result Rocprofiler-systems (https://github.com/ROCm/rocm-systems/tree/develop/projects/rocprofiler-systems) was tested on supported platforms. A small test with Lulesh/Kokkos path was performed and no performance regressions were observed. Details are updated in PR: ROCm/rocm-systems#5406 ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests. --------- Co-authored-by: Kian Cossettini <Kian.Cossettini@amd.com>
## Motivation <!-- Explain the purpose of this PR and the goals it aims to achieve. --> Add CODEOWNERS file to `dyninst_13` branch. CODEOWNER files are required for all public repos and ours was found missing from an org-wide audit ## Technical Details <!-- Explain the changes along with any relevant GitHub links. --> Add default reviewer `ROCm/rocprof-sys` ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.
…#19) ## Motivation **NOTE**: This is a modified version of my PR on dyninst (dyninst#2235) Original issue: dyninst#1290 <!-- Explain the purpose of this PR and the goals it aims to achieve. --> Add support for ELF's RELR dynamic relocation. ## Technical Details Generally, I tried to follow how the existing `RT_REL` and `RT_RELA` were implemented. Of course, `RELR` is a different relocation format that is a compressed relative-relocation table encoded with address and bitmap entries, so it needs separate decode logic. Overall, `elf.h` defines for `RELR` were added and their cases were added in the relevant sections. Also: In `Object-elf` files: - `decodeRelrEntries(...)`: Decodes the entries following https://dram.page/p/relative-relocs-explained/#:~:text=The%20idea%20originates,handled%20needs%20relocating. - `get_relocationRelr_entries(...)`: Finds `.relr.dyn`, determine whether `uintptr_t` is 4 or 8 bytes from the ELF class, validate `DT_RELRENT` + `DT_RELRSZ` against the section data, then call `decodeRelrEntries(...)`. In `emitElf` files: - `.relr.dyn` was not added to `updateLinkInfoSecs` as `sh_info` and `sh_link` are unused. - Within `emitElf<ElfTypes>::driver`: - For every `RELR` entry, adjust the value stored in the memory slot needing load-base adjustment. - In the section that checks for pointer-table sections `(.init_array`, `.fini_array`, etc...), prevent re-applying the offset to an address that `RELR` offset adjustment logic already handled. - Handle emitting `RT_RELR` in `createLoadableSections` - `createRelrRelocationSection(...)`: Rebuilds the new `.relr.dyn` section from the decoded `RELR` relocation locations shifted by `library_adjust`. - Within `createSymbolVersions(...)`, preserve original `.gnu.version_r` version-need entries that were not recreated through symbol version references. This keeps symbolless data such as `GLIBC_ABI_DT_RELR` when it was present in the input binary. **NOTE**: `createRelrRelocationSection(...)` does not recompress bitmap entries. It emits each decoded location as a direct `RELR` entry (which is still valid `RELR` encoding... see https://gabi.xinuos.com/elf/06-reloc.html#:~:text=unit%20is%20relocatable.-,Note,that%20a%20simple%20list%20of%20(even)%20addresses%20is%20a%20valid%20encoding.,-%E2%86%90%205.). **More details can be found in dyninst#2235, including discussions with contributors**. <!-- Explain the changes along with any relevant GitHub links. --> ## Test Plan <!-- Explain any relevant testing done to verify this PR. --> See ROCm/rocm-systems#5736 ## Test Result <!-- Briefly summarize test outcomes. --> When I forced binaries on `rocm-systems` to be compiled with `RELR` relocation, CI workflows passed (excluding Ubuntu Jammy, but that's because GLIBC version is too old). When adding RHEL 10 workflows, the original test failure that started this entire fiasco (`coreutils` binary rewrite) is no longer observed. **Note**: The implemented code only executes when `RELR` relocation is observed (EXCLUDING the preservation of the original `.gnu_version_r` version-need entries that were not created through symbol version references). ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.
## Motivation
`CMAKE_C_FLAGS` and `CMAKE_CXX_FLAGS` are space-separated strings. When
assigned directly to a CMake list variable the entire string becomes a
single
list element, so a value like `-march=native -O2` reaches the compiler
as one
opaque argument instead of two separate flags. This silently breaks
compilation
when any flag requires a following argument (e.g. `-isystem /path`) or
when
the compiler rejects the concatenated form.
This surfaces in practice with Clang, which is stricter about multi-flag
strings than GCC.
## Technical Details
Use `separate_arguments(... UNIX_COMMAND ...)` to tokenize
`CMAKE_C_FLAGS` and
`CMAKE_CXX_FLAGS` into proper CMake list elements before appending them
to the
`DYNINST_C_FLAGS_<BUILD>` / `DYNINST_CXX_FLAGS_<BUILD>` variables:
```cmake
separate_arguments(_dyninst_c_flags UNIX_COMMAND "${CMAKE_C_FLAGS}")
separate_arguments(_dyninst_cxx_flags UNIX_COMMAND "${CMAKE_CXX_FLAGS}")
```
`UNIX_COMMAND` mode matches POSIX shell word-splitting, which is the
correct
semantic for flag strings passed on a compiler command line.
Changed file: `cmake/DyninstOptimization.cmake`
- Add missing "includes" to various headers.
## Test Plan
- [x] Build Dyninst with a multi-flag `CMAKE_C_FLAGS` /
`CMAKE_CXX_FLAGS`
(e.g. `-march=native -O2`) and confirm all flags are passed
individually.
- [x] Build with Clang as the C/C++ compiler and verify no flag-parsing
errors.
- [x] Confirm existing CMake build types (Release, Debug,
RelWithDebInfo) still
produce correct flag sets.
## Test Result
Builds successfully with Clang when `CMAKE_C_FLAGS` / `CMAKE_CXX_FLAGS`
contain space-separated flags.
## Submission Checklist
- [ ] Look over the contributing guidelines at
https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.
## Motivation <!-- Explain the purpose of this PR and the goals it aims to achieve. --> Cherry pick 5d2c62a from https://github.com/dyninst/dyninst This modifies the boundary check in the `find_code_from_data` function from inclusive to a half-open boundary. Without this, it can choose the incorrect `PT_LOAD` segment when `.text` starts exactly at the end address of a previous load segment. ## Technical Details <!-- Explain the changes along with any relevant GitHub links. --> Change the following comparisons from `>=` to `>` - https://github.com/dyninst/dyninst/blob/master/symtabAPI/src/Object-elf.C#L2318 - https://github.com/dyninst/dyninst/blob/master/symtabAPI/src/Object-elf.C#L2320 - https://github.com/dyninst/dyninst/blob/master/symtabAPI/src/Object-elf.C#L2329 ## Test Plan <!-- Explain any relevant testing done to verify this PR. --> PR was tested using the original reproducer that warranted this PR: AIPROFSYST-179. PR was also testing using head of dyninst on a custom walker. ## Test Result <!-- Briefly summarize test outcomes. --> `rocprof-sys-instrument` in binary-rewrite mode no longer crashes. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.
## Fix GCC 15 build errors
Two independent compilation failures surface when building with GCC 15
(tested with GCC 15.2.1). This PR fixes both.
### 1. Missing `<cstdint>` in `framestepper.C`
GCC 15's libstdc++ no longer transitively includes `<cstdint>` through
other standard headers. `stackwalk/src/framestepper.C` uses `uint64_t`
without including it directly, producing:
```
error: 'uint64_t' was not declared in this scope
note: 'uint64_t' is defined in header '<cstdint>'; this is probably
fixable by adding '#include <cstdint>'
```
Fixed by adding the explicit include. (Same class of fix as prior commit
`da38f41`, "include stdint.h and cstdint explicitly for newer gcc
versions".)
### 2. Eager instantiation of `unordered_map` conversion in
`map_builder`
`dyncompat/assign/list_of.hpp`'s `map_builder` declared a non-template
conversion operator:
```cpp
operator std::unordered_map<K, V>() const { ... }
```
A non-template conversion operator has its return type instantiated
during overload resolution, **even when the target is an unrelated type
such as `std::map`**. When `map_list_of(...)` is converted to
`std::map<MachRegister, Register>` (in `RegisterConversion-x86.C`), GCC
15 still instantiates `std::unordered_map<MachRegister, Register>`,
which requires `std::hash<Dyninst::MachRegister>`. No such
specialization exists, so GCC 15's libstdc++ eagerly trips:
```
error: static assertion failed: hash function must be copy constructible
```
Fixed by removing the explicit `unordered_map` operator. The existing
generic `template <typename Map> operator Map()` still handles
`unordered_map` targets, but as a template it is only instantiated when
actually selected — so converting to `std::map` no longer drags in the
broken `unordered_map` instantiation.
### Verification
- Full build completes with GCC 15.2.1.
- Confirmed the two real `dyn_hash_map` (= `std::unordered_map`)
conversions that *do* use the generic template path — `entryNames_IAPI`
and `prefixEntryNames_IAPI` in `common/src/arch-x86.C` — still compile
cleanly (their key types are `enum : unsigned int`, which have built-in
`std::hash`).
- Running `rocprofiler-systems workflows` in
ROCm/rocm-systems#7339.
Cherry pick 4af3fa0 from https://github.com/dyninst/dyninst.git Original PR on dyninst: dyninst#2235 This PR can be considered the complete version of #19. The main additions are: - RELR packing - Loop optimization Currently being tested on ROCm systems: ROCm/rocm-systems#8871
## Motivation `stackwalk/src/ppc-swk.C` uses `uint32_t` and `uint64_t` without including `<cstdint>`. Since GCC 13, libstdc++ no longer transitively provides these types through other standard headers, causing build failures on ppc64le. This file is only compiled on POWER targets, so x86 builds were unaffected. ## Technical Details Add `#include <cstdint>` to `stackwalk/src/ppc-swk.C`. This is the same class of fix as commit 5dff47b ("Fix GCC 15 build failure (#22)"), which addressed missing includes in other Dyninst source files but missed `ppc-swk.C` since it is only compiled on POWER. ## Test Plan - Built Dyninst as part of rocprofiler-systems on ppc64le (IBM POWER10, GCC 15 / Clang 23) — `ppc-swk.C` now compiles without errors. - Verified x86_64 build is unaffected (file is not compiled on x86). ## Test Result Build succeeds on ppc64le where it previously failed with: ``` ppc-swk.C:51:7: error: unknown type name 'uint32_t' ppc-swk.C:55:7: error: unknown type name 'uint64_t' ``` ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests. Co-authored-by: Claude <noreply@anthropic.com>
…he (#29) ## Motivation For background on the issue being fixed, see ROCm/rocm-systems#10171 The problem is that the `DwarfHandle` cache is keyed only on filename, so it hands back a handle bound to an `Elf_X` whose image `~Symtab` already unmapped; this verifies the binding and rebuilds the entry when it differs. ## Technical Details Reuse a cached `DwarfHandle` only when its `origFile()` matches the `Elf_X` being passed in, otherwise drop the stale entry and build a new handle. ## Test Plan ROCm/rocm-systems#10255 (comment) ## Test Result See Test PR on rocprofiler-systems, all tests pass. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.
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
14 participants
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.
Motivation
Update default branch to dyninst_13
Technical Details
Test Plan
Test Result
Submission Checklist