Hi, I just received this bug report via email about a crash in tvtime when running tvtime -v. The crash is in this function: void cpuinfo_print_info( void ) { cpuid_regs_t regs, regs_ext; unsigned int max_ext_cpuid; int family, model, stepping; char idstr[13]; char *model_name; char processor_name[49]; int i; regs = cpuid(0); store32(idstr+0, regs.ebx); store32(idstr+4, regs.edx); store32(idstr+8, regs.ecx); idstr[12] = 0; regs = cpuid( 1 ); family = (regs.eax >> 8) & 0xf; model = (regs.eax >> 4) & 0xf; stepping = regs.eax & 0xf; model_name = table_lookup_model( get_cpu_vendor( idstr ), family, model ); regs_ext = cpuid((1<<31) + 0); max_ext_cpuid = regs_ext.eax; if (max_ext_cpuid >= (1<<31) + 1) { regs_ext = cpuid((1<<31) + 1); if (max_ext_cpuid >= (1<<31) + 4) { for (i = 2; i <= 4; i++) { regs_ext = cpuid((1<<31) + i); store32(processor_name + (i-2)*16, regs_ext.eax); store32(processor_name + (i-2)*16 + 4, regs_ext.ebx); store32(processor_name + (i-2)*16 + 8, regs_ext.ecx); store32(processor_name + (i-2)*16 + 12, regs_ext.edx); } processor_name[48] = 0; model_name = processor_name; } } /* Is this dangerous? */ while( isspace( *model_name ) ) model_name++; fprintf( stderr, "cpuinfo: CPU %s, family %d, model %d, stepping %d.\n", mod fprintf( stderr, "cpuinfo: CPU measured at %.3fMHz.\n", cpuinfo_get_speed() } On the system where the crash happens the code path which does: model_name = processor_name; Is entered and then things segv on the: while( isspace( *model_name ) ) model_name++; Yes right below the is this dangerous comment, but AFAICT that code is fine. Now installing debuginfo and running this under gdb yields the following: Program received signal SIGSEGV, Segmentation fault. 0x0000555555596664 in cpuinfo_print_info () at cpuinfo.c:240 240 while( isspace( *model_name ) ) model_name++; (gdb) p model_name $1 = 0xffffbe50 <error: Cannot access memory at address 0xffffbe50> (gdb) p processor_name $2 = "Intel(R) Core(TM) i5-6500 CPU @ 3.20GHz\000\000\000\000\000\000\000\000\000" (gdb) p &processor_name[0] $3 = 0x7fffffffbe50 "Intel(R) Core(TM) i5-6500 CPU @ 3.20GHz" Notice how model_name only contains the low 32 bits of the processor_name's array address, this looks like a compiler bug to me. One more data point is that this only happens when building tvtime with the Fedora CFLAGS, when building directly from git, which uses: "-g -O3 -fomit-frame-pointer -std=gnu99", the problem is gone. The user reporting this via email mentioned that the following change fixes this: @@ -232,7 +232,7 @@ store32(processor_name + (i-2)*16 + 12, regs_ext.edx); } processor_name[48] = 0; - model_name = processor_name; + model_name = &processor_name; } } But that makes little sense to me, since the 2 are equivalent to the best of my knowledge. I know you are going to need more info, like preprocessed files, etc. But I'm afraid I do not know exactly what you need, so if you let me know I'll provide it to you. Note tvtime is in Fedora and this crash happens with the Fedora pkgs. Regards, Hans
Yeah, I'll certainly need the preprocessed source for the file that contains cpuinfo_print_info, just add -save-temps, plus all gcc options you use. Try -W -Wall if the compiler doesn't tell you something, or -fno-strict-aliasing (dunno what store32 does, but aliasing violation might be what happens there).
Hi, Oh fun, I just did a "fedpkg local" to try the users weird patch, but that build actually works, then I did a "fedpkg mockbuild" and that one does not work. I realize that mockbuild may end up using a different compiler then I've installed but my system is a fully up2date F24 and the mockbuild is for rawhide. And the bug manifests itself in the f23 pkgs too, so it seems to be triggered by some changes between building in mock vs building locally. Digging deeper ... Regards, Hans
Created attachment 1134090 [details] Pre-processed cpuninfo.c Ok, so the fedpkg local build working was a red-herring, it was using ccache and a .o file build with different compiler flags, please ignore. I'm attaching a pre-processed cpuinfo.i, cpuinfo.s and .o all generated with: gcc -DHAVE_CONFIG_H -I. -I.. `/usr/bin/freetype-config --cflags` -Wall -pedantic -I. -DDATADIR="\"/usr/share/tvtime\"" -DCONFDIR="\"/etc/tvtime\"" -DFIFODIR="\"/tmp\"" -D_LARGEFILE64_SOURCE -DLOCALEDIR="\"/usr/share/locale\"" -I../plugins -I/usr/include/libxml2 -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic -std=gnu99 -save-temps -c -o tvtime-cpuinfo.o cpuinfo.c Which is the exact same command as used during a fedpkg local / mockbuild and which results in a .o file with the problem.
Created attachment 1134091 [details] Working cpuinfo.s build with gcc -save-temps
Created attachment 1134092 [details] Working cpuinfo.o build with -save-temps
Oops, please do not waste time looking at these, for some reason if I link with the .o file build this way the problem is gone.
Ok, so this is quite weird, adding -save-temps to the CFLAGS seems to fix the problem ? I've made the following change to the .spec file: %build +export CFLAGS="$RPM_OPT_FLAGS -save-temps" %configure --disable-dependency-tracking --disable-rpath -make %{?_smp_mflags} +make V=1 %install With ccache taken out of the equation (double checked) and when build this way all is well. Note I had to remove %{?_smp_mflags} because that does not play well with -save-temps. I've also tried building with just %{?_smp_mflags} removed to rule that out but that does not help. The only difference between a working and non-working "fedpkg local" is the addition of "-save-temps" to the CFLAGS of the working build. On a hunch I've also tried stripping -pipe from the CFLAGS, that does not help. Any clues appreciated, note the attached .s / . o file were made with -save-temps so they are from a *working* build.
Created attachment 1134101 [details] NON working cpuinfo.o build without -save-temps Here is a cpuinfo.o build with the exact same command as given in comment #3 but then with -save-temps omitted, linking this into the tvtime binary results in a broken build.
So, can you make sure ccache is uninstalled, and try compiling that cpuinfo.c by hand with the flags from the log file? Once with -S -g0 without -save-temps, once with -S -save-temps -g0, check which version then works/fails and check how they are different? Going through separate -E and compilation has some differences, but mostly to locations of different tokens, so can affect usually primarily debug info, or could affect some warnings. Also, if -S -g0 without -save-temps fails and -S -g0 -save-temps works, can you try -E -fdirectives-only preprocessing and then compiling it with -S -g0?
Jakub, I've just double checked all my findings: 0) Check PATH make sure ccache is not in there 1) Build by hand with gcc command from comment 3, link -> works. 2) Download attached "Working cpuinfo.o" compare with one just build -> identical. 3) Build by hand with gcc command from comment 3 with -save-temps removed, link -> Does NOT work 4) Download attached "NON working cpuinfo.o" compare with one just build -> identical. I'll try what you've asked for in comment 9 next. Thanks & Regards, Hans
Created attachment 1134107 [details] cpuinfo.c build with -S without -save-temps This is the result of running: gcc -DHAVE_CONFIG_H -I. -I.. `/usr/bin/freetype-config --cflags` -Wall -pedantic -I. -DDATADIR="\"/usr/share/tvtime\"" -DCONFDIR="\"/etc/tvtime\"" -DFIFODIR="\"/tmp\"" -D_LARGEFILE64_SOURCE -DLOCALEDIR="\"/usr/share/locale\"" -I../plugins -I/usr/include/libxml2 -O2 -g0 -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic -std=gnu99 -S cpuinfo.c Using this by calling: gcc -o tvtime-cpuinfo.o -c cpuinfo.s make # to link Results in a non working binary
Created attachment 1134108 [details] cpuinfo.c build with -S with -save-temps This is the result of running: gcc -DHAVE_CONFIG_H -I. -I.. `/usr/bin/freetype-config --cflags` -Wall -pedantic -I. -DDATADIR="\"/usr/share/tvtime\"" -DCONFDIR="\"/etc/tvtime\"" -DFIFODIR="\"/tmp\"" -D_LARGEFILE64_SOURCE -DLOCALEDIR="\"/usr/share/locale\"" -I../plugins -I/usr/include/libxml2 -O2 -g0 -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic -std=gnu99 -save-temps -S cpuinfo.c Using this by calling: gcc -o tvtime-cpuinfo.o -c cpuinfo.s make # to link Results in a working binary
Created attachment 1134109 [details] cpuinfo.c build with -E -fdirectives-only This is the result of running: gcc -DHAVE_CONFIG_H -I. -I.. `/usr/bin/freetype-config --cflags` -Wall -pedantic -I. -DDATADIR="\"/usr/share/tvtime\"" -DCONFDIR="\"/etc/tvtime\"" -DFIFODIR="\"/tmp\"" -D_LARGEFILE64_SOURCE -DLOCALEDIR="\"/usr/share/locale\"" -I../plugins -I/usr/include/libxml2 -O2 -g0 -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic -std=gnu99 -E -fdirectives-only cpuinfo.c -o cpuinfo.i.c
Created attachment 1134111 [details] cpuinfo.i.c build with -g0 -S This is the result of running: gcc -DHAVE_CONFIG_H -I. -I.. `/usr/bin/freetype-config --cflags` -Wall -pedantic -I. -DDATADIR="\"/usr/share/tvtime\"" -DCONFDIR="\"/etc/tvtime\"" -DFIFODIR="\"/tmp\"" -D_LARGEFILE64_SOURCE -DLOCALEDIR="\"/usr/share/locale\"" -I../plugins -I/usr/include/libxml2 -O2 -g0 -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic -std=gnu99 -S cpuinfo.i.c On the previously attached cpuinfo.i.c . Using this by calling: gcc -o tvtime-cpuinfo.o -c cpuinfo.i.s make # to link Results in a working binary
p.s. I've done "rpm -e ccache" before doing all the stuff from comment 11 and later, and I've repeated the tests from comment 10 with this, nothing chaned (no surprise not having ccache in PATH should suffice).
Ah, ok; so there are 2 bugs: 1) in tvtime's cpuinfo.c: static cpuid_regs_t cpuid( int func ) { cpuid_regs_t regs; #define CPUID ".byte 0x0f, 0xa2; " #if !defined(PIC) && !defined(__PIC__) asm("movl %4,%%eax; " CPUID "movl %%eax,%0; movl %%ebx,%1; movl %%ecx,%2; movl %%edx,%3" : "=m" (regs.eax), "=m" (regs.ebx), "=m" (regs.ecx), "=m" (regs.edx) : "g" (func) : "%eax", "%ebx", "%ecx", "%edx"); #else asm("movl %%ebx, %%esi; movl %4,%%eax; " CPUID "movl %%eax,%0; movl %%ebx,%1; movl %%ecx,%2; movl %%edx,%3; movl %%esi, %%ebx" : "=m" (regs.eax), "=m" (regs.ebx), "=m" (regs.ecx), "=m" (regs.edx) : "g" (func) : "%eax", "%ecx", "%edx", "%esi"); #endif return regs; } The PIC case is of course broken for x86-64, it claims that it doesn't clobber the bx register, but as it saves/restores only the low 32-bits of that register, it clears the upper 32-bits of that. Note that the PIC case is really not needed for gcc 5.x and later, even in 32-bit code and for 64-bit code with medium/large pic models the recent gcc versions can cope with just clobbering of "%ebx". For GCC 4.9 and earlier, the PIC code is indeed needed for 32-bit PIC/PIE, or for defined(__x86_64__) && (defined(__code_model_medium__) || \ defined(__code_model_large__)) && defined(__PIC__) but of course you can't use the exact same asm - you need to save/restore %rbx using movq %%rbx, %%rsi and back. The asm looks quite inefficient, but as this isn't performance critical code, no need to replace it with something better. So, if you want minimal change, just have one version of the PIC code for ifndef __x86_64__ and another one for __x86_64__. 2) the reason for why -save-temps works and without it it doesn't is a bug in redhat-rpm-config, /usr/lib/rpm/redhat/redhat-hardened-cc1 contains just *cc1_options: + %{!fpie:%{!fPIE:%{!fpic:%{!fPIC:%{!fno-pic:-fPIE}}}}} but it really should contain it also for *cpp_options: + %{!fpie:%{!fPIE:%{!fpic:%{!fPIC:%{!fno-pic:-fPIE}}}}} because without that, the implicit -fPIE is passed only when compiling, and if preprocessing separately %(cc1_options) is not passed to the first cc1 invocation with -E, so in your case you get the __PIC__ and __PIE__ defined when using integrated preprocessor with compilation, but not when preprocessing. Feel free to file redhat-rpm-config bug.
Hi, thanks for the debugging and analysis. Just to be sure, the patch should look like this (replace the first and last movl with movq on 64bit): +#ifndef __x86_64__ asm("movl %%ebx, %%esi; movl %4,%%eax; " CPUID "movl %%eax,%0; movl %%ebx,%1; movl %%ecx,%2; movl %%edx,%3; movl %%esi, %%ebx" : "=m" (regs.eax), "=m" (regs.ebx), "=m" (regs.ecx), "=m" (regs.edx) : "g" (func) : "%eax", "%ecx", "%edx", "%esi"); +#else + asm("movq %%ebx, %%esi; movl %4,%%eax; " CPUID + "movl %%eax,%0; movl %%ebx,%1; movl %%ecx,%2; movl %%edx,%3; movq %%esi, %%ebx" + : "=m" (regs.eax), "=m" (regs.ebx), "=m" (regs.ecx), "=m" (regs.edx) + : "g" (func) + : "%eax", "%ecx", "%edx", "%esi"); +#endif
No, the above won't assemble. movq needs to be movq %%rbx, %%rsi; and movq %%rsi, %%rbx. Otherwise it will work, inefficient, but who cares.
(In reply to Jakub Jelinek from comment #18) > No, the above won't assemble. movq needs to be movq %%rbx, %%rsi; and movq > %%rsi, %%rbx. > Otherwise it will work, inefficient, but who cares. Ah. OK. Thanks a lot.
Thanks Jakub! Testing the fix right now, and preparing an update fixing this.
tvtime-1.0.10-2.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2016-da412774c1
tvtime-1.0.10-2.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report. See https://fedoraproject.org/wiki/QA:Updates_Testing for instructions on how to install test updates. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-da412774c1
tvtime-1.0.10-2.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.
tvtime-1.0.10-2.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2016-d036f65c21
tvtime-1.0.10-2.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report. See https://fedoraproject.org/wiki/QA:Updates_Testing for instructions on how to install test updates. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-d036f65c21
tvtime-1.0.10-2.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.