First attempt to send this failed.
On Fri, Sep 12, 2014 at 3:41 PM, Caroline Tice <cmt...@google.com> wrote: > > Hi Patrick, > > Mostly your patch looks OK to me, though there are a couple of serious issues > (mentioned below). Most of my comments are for formatting stuff. Once you > have fixed these issues, let me know and I'll look at it again. But someone > else will still have to approve the parts of this patch that are outside the > libvtv directory (I believe). > > -- Caroline Tice > cmt...@google.com > > > In changes to gcc/config/i386/cygwin.h mingw-w64.h and mingw32.h, you forgot > to handle the "fvtable-verify=preinit" options. fvtable-veriy=preinit should > cause vtv_start_preinit.o to be added to the STARTFILE_SPEC and > vtv_end_preinit.o to be added to the ENDFILE_SPEC (as in > gcc/config/gnu-user.h). I expect you will also need to add it to your > LIB_SPEC definitions in those config files. > > in libgcc/config.host, the indentation looks wrong on the line 660 (where you > add the extra parts for vtable verification for the case > "i[34567]86-*-mingw*)". It also looks wrong on line 709 (again, adding extra > parts, for case "x86_64-*-mingw*)". The rest of the changes in that file > look ok, but someone else will need to approve them. > > The changes in libgcc/Makefile.in and gcc/varasm.c look ok to me, but someone > will will have to approve them since I don't have authority to approve > changes there. > > in libstdc++-v3/libsupc++: > > Your change in Makefile.am looks ok to me; your changes in vtv_stubs.cc look > ok, except that you appear to have messed up the indentations in the function > headers (both for the declarations and the actual functions). Also there is > a typo in your comment: 'build' should be 'built'. But content-wise, the > change looks fine. Again, someone else will have to actually approve these > changes since I do not have that authority. > > in libstdc++-v3/src/Makefile.am also looks ok to me; someone else will have > to give final approval. > > > in libvtv/Makefile.am, you need to fix the indentation at line 64 > (vtv_stubs.cc): > > vtv_stubs_sources = \ > vtv_start.c \ > vtv_stubs.cc \ > vtv_end.c > > the rest of the changes in that file look good. > > Why did you make a copy of obstack.c in libvtv rather than using the one in > libiberty? It would be better not to make a second copy of the source file > if that can be avoided... > > in libvtv/vtv_malloc.cc: > > lines 207-213: Fix the indentation on the second line of call to > VirtualAlloc. > #if defined (__CYGWIN__) || defined (__MINGW32__) > if ((allocated = VirtualAlloc(NULL, size, MEM_RESERVE|MEM_COMMIT, > PAGE_READWRITE)) == 0) > #else > if ((allocated = mmap (NULL, size, PROT_READ | PROT_WRITE," > MAP_PRIVATE | MAP_ANONYMOUS, -1, 0)) == 0) > #endif > > Remove extra blank line at line 216. > > in libvtv/vtv_rts.cc: > > Your version of the function read_section_offset_and_length has several lines > that exceed the 80 character limit; please fix that. Your function > iterate_modules also has one or two lines that are too long, and it needs a > function comment. > > in libvtv/vtv_utils.cc: > > In the function __vtv_open_log, you seem to have the following code twice: > > #ifdef __MINGW32__ > mkdir (logs_prefix); > #else > mkdir (logs_prefix, S_IRWXU); > #endif > > was there a reason for this, or is this an accident (in which case the second > occurrence should be removed)? > > > > On Wed, Sep 10, 2014 at 9:12 PM, Patrick Wollgast <patrick.wollg...@rub.de> > wrote: >> >> Ping for https://gcc.gnu.org/ml/gcc-patches/2014-08/msg02559.html >> >> Also added Caroline Tice, as libvtv maintainer, to cc and attached >> virtual_func_test_min_UAF.cpp, which I forgot in the original mail. >> >> Patrick >> >> On 28.08.2014 13:03, Patrick Wollgast wrote: >> > This patch contains a port of VTV -fvtable-verify=std for Cygwin and MinGW. >> > >> > Since weak symbols on Windows and Linux are implemented differently, and >> > VTV should have the possibility to be switched on and off, the structure >> > of the feature had to be modified. >> > On Linux libstdc++ contains the weak stub functions of VTV. For Cygwin >> > and MinGW they have been removed, due to the difference of weak symbols. >> > On Linux and on Windows libstdc++ itself gets build with >> > -fvtable-verify=std. Since libvtv gets build after libstdc++, and >> > libstdc++ doesn't contain the stub functions any more, 'undefined >> > reference' errors are thrown during linking of libstdc++. To prevent >> > these errors during the linking process a libvtv-0.dll gets build from >> > the stub functions before libstdc++-6.dll is linked. >> > At the end of the build process two VTV dlls have been build. One is >> > called libvtv-0.dll, containing the real functions, the other is called >> > libvtv_stubs-0.dll, containing the stub functions. Depending on whether >> > libvtv-0.dll is first found in the dll search path or >> > libvtv_stubs-0.dll, renamed to libvtv-0.dll, the real functions or the >> > stub functions are used. >> > >> > Testing: >> > The test builds were configured the following way: >> > Linux 64bit (from patched and unpatched trunk): >> > /path/to/configure --prefix=/prefix/gcc-vtv-bin-64 >> > --enable-libstdcxx-threads --enable-vtable-verify=yes >> > MinGW 32bit cross compiled: >> > /path/to/configure --target=i686-w64-mingw32 >> > --prefix=/prefix/mingw-vtv-bin-32 --with-gnu-ld --with-gnu-as >> > --enable-fully-dynamic-string --disable-multilib >> > --enable-libstdcxx-threads --enable-vtable-verify=yes >> > MinGW 64bit cross compiled: >> > /path/to/configure --target=x86_64-w64-mingw32 >> > --prefix=/prefix/mingw-vtv-bin-64 --with-gnu-ld --with-gnu-as >> > --enable-fully-dynamic-string --disable-multilib >> > --enable-libstdcxx-threads --enable-vtable-verify=yes >> > Cygwin 64bit: >> > /path/to/configure --enable-languages=c,c++ --enable-libstdcxx-threads >> > --enable-vtable-verify=yes >> > >> > At Linux the patched and unpatched version resulted in the same number >> > of passed tests with 'make check-target-libvtv'. >> > >> > Since MinGW was cross compiled the test cases couldn't be built and run >> > with 'make check-target-libvtv'. Therefore they were built with the >> > attached makefiles and tested afterwards on Windows 7 64bit. Some test >> > cases contain Linux specific parts and weren't tested. See the makefiles >> > for further information. Additionally virtual_func_test_min_UAF.cpp was >> > also built and tested. All built tests passed. >> > >> > Cygwin was just tested on gcc 4.9.0, because the current trunk isn't >> > building for me. Even the clean trunk without the patch attached to this >> > mail. On Cygwin with gcc 4.9.0 VTV worked. >> > >> > Besides the test cases Botan was also built and tested (gcc 4.9.0) with >> > MinGW 32bit and VTV. >> > >> > regards >> > >> >> -- >> Beste Grüße, >> Patrick > >
