Skip to content

Commit ddd1f57

Browse files
committed
noinline to avoid compiler reading TOC before PATCHER_BEGIN
This bug was first seen in a different product that's using the same interception code as OMPI. But I think it's potentially in OMPI too. In my vanilla build of OMPI master on RH8 if I "gdb libopen-pal.so" and "disassemble intercept_brk", I'm seeing a suspicious extra instruction in front of PATCHER_BEGIN: 0x00000000000d6778 <+40>: std r2,24(r1) // something gcc put in front 0x00000000000d677c <+44>: std r2,96(r1) // PATCHER_BEGIN's toc_save 0x00000000000d6780 <+48>: nop // NOPs from PATCHER_BEGIN 0x00000000000d6784 <+52>: nop // that get replaced 0x00000000000d6788 <+56>: nop // by instructions that 0x00000000000d678c <+60>: nop // change r2 0x00000000000d6790 <+64>: nop // Later there are loads from that location like 0x000000000019e0e4 <+132>: ld r2,24(r1) that make me nervous since that's the pre-updated value. I believe this is the same thing Nathan is describing way back in a9bc692 and his solution was to put a second call around each interception, where the outer call is just intercept_brk(): PATCHER_BEGIN _intercept_brk() PATCHER_END and the inner call _intercept_brk() is where the bulk of the code goes. What I'm seeing is that _intercept_brk() is being inlined and probably negating Nathan's fix. So I want to add __opal_attribute_noinline__ to restore the fix. With this commit in place, the disassembly of intercept_brk becomes tiny because it's no longer inlining _intercept_brk() and the susipicious early save of r2 is gone. I made the same fix to all the intercept_* functions, although intercept_brk was the only one that had a suspicious save of r2. As far as empirical failures though, we only have those from the non-OMPI product that's using the same patcher code. I'm not actually getting OMPI to fail from the above suspicious data being saved in r1+24. Signed-off-by: Mark Allen <markalle@us.ibm.com>
1 parent 0a21a58 commit ddd1f57

File tree

1 file changed

+16
-0
lines changed

1 file changed

+16
-0
lines changed

opal/mca/memory/patcher/memory_patcher_component.c

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,22 @@ opal_memory_patcher_component_t mca_memory_patcher_component = {
107107
* data. If this can be resolved the two levels can be joined.
108108
*/
109109

110+
/*
111+
* Nathan's original fix described above can have the same problem reappear if the
112+
* interception functions inline themselves.
113+
*/
114+
static void *_intercept_mmap(void *start, size_t length, int prot, int flags, int fd, off_t offset) __opal_attribute_noinline__;
115+
static int _intercept_munmap(void *start, size_t length) __opal_attribute_noinline__;
116+
#if defined(__linux__)
117+
static void *_intercept_mremap (void *start, size_t oldlen, size_t newlen, int flags, void *new_address) __opal_attribute_noinline__;
118+
#else
119+
static void *_intercept_mremap (void *start, size_t oldlen, void *new_address, size_t newlen, int flags) __opal_attribute_noinline__;
120+
#endif
121+
static int _intercept_madvise (void *start, size_t length, int advice) __opal_attribute_noinline__;
122+
static int _intercept_brk (void *addr) __opal_attribute_noinline__;
123+
static void *_intercept_shmat(int shmid, const void *shmaddr, int shmflg) __opal_attribute_noinline__;
124+
static int _intercept_shmdt (const void *shmaddr) __opal_attribute_noinline__;
125+
110126
#if defined (SYS_mmap)
111127

112128
#if defined(HAVE___MMAP) && !HAVE_DECL___MMAP

0 commit comments

Comments
 (0)