| Message ID | alpine.BSF.2.00.1212090306300.64779@toaster.local (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Alex Netes |
| Headers | show |
On 12/09/12 12:07, Garrett Cooper wrote: > It seems that there's a bug when linking inlined functions with clang; this > issue will need to be upstreamed and reverified with clang 3.2. > > Signed-off-by: Garrett Cooper <yanegomi@gmail.com> > --- > osmtest/osmtest.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/osmtest/osmtest.c b/osmtest/osmtest.c > index 68b0e40..4f56106 100644 > --- a/osmtest/osmtest.c > +++ b/osmtest/osmtest.c > @@ -3068,7 +3068,7 @@ Exit: > return (status); > } > > -inline uint32_t osmtest_path_rec_key_get(IN const ib_path_rec_t * const p_rec) > +static uint32_t osmtest_path_rec_key_get(IN const ib_path_rec_t * const p_rec) > { > return (p_rec->dlid << 16 | p_rec->slid); > } Why was the "inline" keyword dropped ? You didn't explain that in the patch description. Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Dec 9, 2012 at 5:23 AM, Bart Van Assche <bvanassche@acm.org> wrote: > On 12/09/12 12:07, Garrett Cooper wrote: >> It seems that there's a bug when linking inlined functions with clang; this >> issue will need to be upstreamed and reverified with clang 3.2. >> >> Signed-off-by: Garrett Cooper <yanegomi@gmail.com> >> --- >> osmtest/osmtest.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/osmtest/osmtest.c b/osmtest/osmtest.c >> index 68b0e40..4f56106 100644 >> --- a/osmtest/osmtest.c >> +++ b/osmtest/osmtest.c >> @@ -3068,7 +3068,7 @@ Exit: >> return (status); >> } >> >> -inline uint32_t osmtest_path_rec_key_get(IN const ib_path_rec_t * const p_rec) >> +static uint32_t osmtest_path_rec_key_get(IN const ib_path_rec_t * const p_rec) >> { >> return (p_rec->dlid << 16 | p_rec->slid); >> } > > Why was the "inline" keyword dropped ? You didn't explain that in the patch description. "inline" was replaced with "static". Reason being (again, although I didn't explain in complete detail -- sorry, it was late when I sent out the patch) is that there's a compiler/linker bug in clang where it appears to optimize out the function improperly on 3.0 at least, but not fully because it ends up confusing the linker. So I've sidestepped the issue by making the function static (so it could be better optimized by the compiler) instead of inlined. An example is provided below. Thanks, -Garrett # cat /etc/redhat-release Fedora release 17 (Beefy Miracle) # uname -a Linux fallout-fedora.local 3.6.9-2.fc17.i686 #1 SMP Tue Dec 4 14:22:00 UTC 2012 i686 i686 i386 GNU/Linux # clang --version clang version 3.0 (tags/RELEASE_30/final) Target: i386-redhat-linux-gnu Thread model: posix # env CC=clang CXX=clang++ CFLAGS="-Werror -I/nfs/bayonetta/scratch/git/ofed/bin/include" LDFLAGS="-L/nfs/bayonetta/scratch/git/ofed/bin/lib" ./configure --prefix=$PWD/../bin ... # make clean ... # make all ... Making all in osmtest make[1]: Entering directory `/nfs/bayonetta/scratch/git/ofed/opensm/osmtest' clang -DHAVE_CONFIG_H -I. -I../include -I../include/opensm -I./include -I./../include -I./../../libibumad/include -I/nfs/bayonetta/scratch/git/ofed/opensm/../bin/include -Wall -g -Werror -I/nfs/bayonetta/scratch/git/ofed/bin/include -MT osmtest-main.o -MD -MP -MF .deps/osmtest-main.Tpo -c -o osmtest-main.o `test -f 'main.c' || echo './'`main.c mv -f .deps/osmtest-main.Tpo .deps/osmtest-main.Po clang -DHAVE_CONFIG_H -I. -I../include -I../include/opensm -I./include -I./../include -I./../../libibumad/include -I/nfs/bayonetta/scratch/git/ofed/opensm/../bin/include -Wall -g -Werror -I/nfs/bayonetta/scratch/git/ofed/bin/include -MT osmtest-osmtest.o -MD -MP -MF .deps/osmtest-osmtest.Tpo -c -o osmtest-osmtest.o `test -f 'osmtest.c' || echo './'`osmtest.c mv -f .deps/osmtest-osmtest.Tpo .deps/osmtest-osmtest.Po clang -DHAVE_CONFIG_H -I. -I../include -I../include/opensm -I./include -I./../include -I./../../libibumad/include -I/nfs/bayonetta/scratch/git/ofed/opensm/../bin/include -Wall -g -Werror -I/nfs/bayonetta/scratch/git/ofed/bin/include -MT osmtest-osmt_service.o -MD -MP -MF .deps/osmtest-osmt_service.Tpo -c -o osmtest-osmt_service.o `test -f 'osmt_service.c' || echo './'`osmt_service.c mv -f .deps/osmtest-osmt_service.Tpo .deps/osmtest-osmt_service.Po clang -DHAVE_CONFIG_H -I. -I../include -I../include/opensm -I./include -I./../include -I./../../libibumad/include -I/nfs/bayonetta/scratch/git/ofed/opensm/../bin/include -Wall -g -Werror -I/nfs/bayonetta/scratch/git/ofed/bin/include -MT osmtest-osmt_slvl_vl_arb.o -MD -MP -MF .deps/osmtest-osmt_slvl_vl_arb.Tpo -c -o osmtest-osmt_slvl_vl_arb.o `test -f 'osmt_slvl_vl_arb.c' || echo './'`osmt_slvl_vl_arb.c mv -f .deps/osmtest-osmt_slvl_vl_arb.Tpo .deps/osmtest-osmt_slvl_vl_arb.Po clang -DHAVE_CONFIG_H -I. -I../include -I../include/opensm -I./include -I./../include -I./../../libibumad/include -I/nfs/bayonetta/scratch/git/ofed/opensm/../bin/include -Wall -g -Werror -I/nfs/bayonetta/scratch/git/ofed/bin/include -MT osmtest-osmt_multicast.o -MD -MP -MF .deps/osmtest-osmt_multicast.Tpo -c -o osmtest-osmt_multicast.o `test -f 'osmt_multicast.c' || echo './'`osmt_multicast.c mv -f .deps/osmtest-osmt_multicast.Tpo .deps/osmtest-osmt_multicast.Po clang -DHAVE_CONFIG_H -I. -I../include -I../include/opensm -I./include -I./../include -I./../../libibumad/include -I/nfs/bayonetta/scratch/git/ofed/opensm/../bin/include -Wall -g -Werror -I/nfs/bayonetta/scratch/git/ofed/bin/include -MT osmtest-osmt_inform.o -MD -MP -MF .deps/osmtest-osmt_inform.Tpo -c -o osmtest-osmt_inform.o `test -f 'osmt_inform.c' || echo './'`osmt_inform.c mv -f .deps/osmtest-osmt_inform.Tpo .deps/osmtest-osmt_inform.Po /bin/sh ../libtool --tag=CC --mode=link clang -Wall -g -Werror -I/nfs/bayonetta/scratch/git/ofed/bin/include -L/nfs/bayonetta/scratch/git/ofed/bin/lib -o osmtest osmtest-main.o osmtest-osmtest.o osmtest-osmt_service.o osmtest-osmt_slvl_vl_arb.o osmtest-osmt_multicast.o osmtest-osmt_inform.o -L../complib -losmcomp -L../libvendor -losmvendor -L../opensm -lopensm -L/nfs/bayonetta/scratch/git/ofed/opensm/osmtest/../../libibumad/.libs -L/nfs/bayonetta/scratch/git/ofed/opensm/../bin/lib -libumad -libumad -ldl -lpthread libtool: link: clang -Wall -g -Werror -I/nfs/bayonetta/scratch/git/ofed/bin/include -o osmtest osmtest-main.o osmtest-osmtest.o osmtest-osmt_service.o osmtest-osmt_slvl_vl_arb.o osmtest-osmt_multicast.o osmtest-osmt_inform.o -L/nfs/bayonetta/scratch/git/ofed/bin/lib -L../complib -L../libvendor /nfs/bayonetta/scratch/git/ofed/opensm/../bin/lib/libosmvendor.so /nfs/bayonetta/scratch/git/ofed/opensm/../bin/lib/libosmcomp.so -L../opensm /nfs/bayonetta/scratch/git/ofed/opensm/../bin/lib/libopensm.so -L/nfs/bayonetta/scratch/git/ofed/opensm/osmtest/../../libibumad/.libs -L/nfs/bayonetta/scratch/git/ofed/opensm/../bin/lib -libumad -ldl -lpthread -Wl,-rpath -Wl,/nfs/bayonetta/scratch/git/ofed/opensm/../bin/lib -Wl,-rpath -Wl,/nfs/bayonetta/scratch/git/ofed/opensm/../bin/lib osmtest-osmtest.o: In function `osmtest_validate_path_rec': /nfs/bayonetta/scratch/git/ofed/opensm/osmtest/osmtest.c:3930: undefined reference to `osmtest_path_rec_key_get' osmtest-osmtest.o: In function `osmtest_parse_path': /nfs/bayonetta/scratch/git/ofed/opensm/osmtest/osmtest.c:6853: undefined reference to `osmtest_path_rec_key_get' clang: error: linker command failed with exit code 1 (use -v to see invocation) [err_drv_command_failed] make[1]: *** [osmtest] Error 1 make[1]: Leaving directory `/nfs/bayonetta/scratch/git/ofed/opensm/osmtest' make: *** [all-recursive] Error 1 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Dec 9, 2012 at 11:06 AM, Garrett Cooper <yanegomi@gmail.com> wrote:
...
It's still an issue with clang 3.2; going to file a bug upstream,
but the attached patch allows the linker stage to go through properly.
Thanks,
-Garrett
# clang -o /root/test_inline /root/test_inline.c
/tmp/test_inline-JRJ1GD.o: In function `foo':
/root/test_inline.c:(.text+0x36): undefined reference to `bar'
/root/test_inline.c:(.text+0x5f): undefined reference to `bar'
/root/test_inline.c:(.text+0x88): undefined reference to `bar'
/root/test_inline.c:(.text+0xb1): undefined reference to `bar'
clang: error: linker command failed with exit code 1 (use -v to see invocation)
# clang
clang clang++ clang-cpp clang-tblgen
# clang --version
FreeBSD clang version 3.2 (branches/release_32 168974) 20121130
Target: i386-unknown-freebsd10.0
Thread model: posix
# cat /root/test_inline.c
#include <stdio.h>
inline int
bar(int i)
{
return (42 < i && i < 84 ? i : -1);
}
static void
foo(void)
{
printf("-4: %d\n", bar(-4));
printf("41: %d\n", bar(41));
printf("43: %d\n", bar(43));
printf("85: %d\n", bar(85));
}
int
main(void)
{
foo();
return (0);
}
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Dec 9, 2012, at 11:28 AM, Garrett Cooper wrote: > On Sun, Dec 9, 2012 at 11:06 AM, Garrett Cooper <yanegomi@gmail.com> wrote: > > ... > > It's still an issue with clang 3.2; going to file a bug upstream, > but the attached patch allows the linker stage to go through properly. > Thanks, > -Garrett > > # clang -o /root/test_inline /root/test_inline.c > /tmp/test_inline-JRJ1GD.o: In function `foo': > /root/test_inline.c:(.text+0x36): undefined reference to `bar' > /root/test_inline.c:(.text+0x5f): undefined reference to `bar' > /root/test_inline.c:(.text+0x88): undefined reference to `bar' > /root/test_inline.c:(.text+0xb1): undefined reference to `bar' > clang: error: linker command failed with exit code 1 (use -v to see invocation) > # clang > clang clang++ clang-cpp clang-tblgen > # clang --version > FreeBSD clang version 3.2 (branches/release_32 168974) 20121130 > Target: i386-unknown-freebsd10.0 > Thread model: posix > # cat /root/test_inline.c > #include <stdio.h> > > inline int > bar(int i) > { > > return (42 < i && i < 84 ? i : -1); > } > > static void > foo(void) > { > > printf("-4: %d\n", bar(-4)); > printf("41: %d\n", bar(41)); > printf("43: %d\n", bar(43)); > printf("85: %d\n", bar(85)); > } > > int > main(void) > { > > foo(); > return (0); > } After talking with some other more knowledgeable people than myself, it looks like opensm is depending upon gcc inline behavior: http://freebsd.1045724.n5.nabble.com/C99-inlines-td4157849.html , http://wiki.freebsd.org/PortsAndClang (look for "undefined or duplicate symbols while linking"). The previously attached patch should be ok for dealing with the linking problem as it just changes the scoping. Definitely learned a few things today about gcc :). Thanks, -Garrett-- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Dec 09, 2012 at 01:47:03PM -0800, Garrett Cooper wrote: > After talking with some other more knowledgeable people than > myself, it looks like opensm is depending upon gcc inline > behavior: > http://freebsd.1045724.n5.nabble.com/C99-inlines-td4157849.html > , http://wiki.freebsd.org/PortsAndClang (look for "undefined > or duplicate symbols while linking"). The previously attached > patch should be ok for dealing with the linking problem as it > just changes the scoping. Definitely learned a few things > today about gcc :). It is definately a bug in the code, it should be 'static inline' - the reason it doesn't blow up on gcc is most likely because nobody ever compiled it with a low enough optimization level to see it. Looks to me like clang is fine, no bug there. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Dec 9, 2012 at 10:06 PM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote: ... > It is definately a bug in the code, it should be 'static inline' - the > reason it doesn't blow up on gcc is most likely because nobody ever > compiled it with a low enough optimization level to see it. > > Looks to me like clang is fine, no bug there. I've been corrected by several people both in the FreeBSD and clang projects today. The summary is: 1. What clang's doing is correct via c99. static inline is the most correct way to do this while inline'ing the function per-c99 (which I didn't understand until today because I was unaware of that point in the standard). In particular: i. With -O0 and -O1 it will fail as noted before. ii. With -O2 and higher it will optimize out without error (which could account for me not seeing this until I ran the build on Fedora 17 as I haven't installed a make.conf with non-default CFLAGS and didn't call the Makefiles with any optimization settings). 2. gcc silently optimizes it out. This is a gcc extension to c89 and c99 (aka gnu89 and gnu99). Thanks! -Garrett -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Dec 9, 2012 at 10:12 PM, Garrett Cooper <yanegomi@gmail.com> wrote:
...
Patch incoming soon that's pedantically correct in terms of
content and the description.
Thanks,
-Garrett
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/osmtest/osmtest.c b/osmtest/osmtest.c index 68b0e40..4f56106 100644 --- a/osmtest/osmtest.c +++ b/osmtest/osmtest.c @@ -3068,7 +3068,7 @@ Exit: return (status); } -inline uint32_t osmtest_path_rec_key_get(IN const ib_path_rec_t * const p_rec) +static uint32_t osmtest_path_rec_key_get(IN const ib_path_rec_t * const p_rec) { return (p_rec->dlid << 16 | p_rec->slid); }
It seems that there's a bug when linking inlined functions with clang; this issue will need to be upstreamed and reverified with clang 3.2. Signed-off-by: Garrett Cooper <yanegomi@gmail.com> --- osmtest/osmtest.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)