Re: [PATCH/Merge Request] Vtable Verification feature.
On Mon, 12 Aug 2013, Caroline Tice wrote: I have finished my first pass at the porting documentation. It is now available on the vtable verification feature web site: http://gcc.gnu.org/wiki/vtv Please provide the links there in a form that does not require JavaScript to read them (in particular, does not require running non-free JavaScript, as per GNU Project principles). That's certainly possible for Google Docs documents; you can read https://docs.google.com/document/pub?id=1ZfyfkB62EFaR4_g4JKm4--guz3vxm9pciOBziMHTnK4pli=1 (GCC Architectural Goals) without JavaScript, for example. -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH/Merge Request] Vtable Verification feature.
I was not aware that the links required Javascript. Can anybody tell me how to fix them so that they don't? -- Caroline Tice cmt...@google.com On Thu, Aug 22, 2013 at 9:58 AM, Joseph S. Myers jos...@codesourcery.com wrote: On Mon, 12 Aug 2013, Caroline Tice wrote: I have finished my first pass at the porting documentation. It is now available on the vtable verification feature web site: http://gcc.gnu.org/wiki/vtv Please provide the links there in a form that does not require JavaScript to read them (in particular, does not require running non-free JavaScript, as per GNU Project principles). That's certainly possible for Google Docs documents; you can read https://docs.google.com/document/pub?id=1ZfyfkB62EFaR4_g4JKm4--guz3vxm9pciOBziMHTnK4pli=1 (GCC Architectural Goals) without JavaScript, for example. -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH/Merge Request] Vtable Verification feature.
I believe I have figured this out and fixed the problem. If you are still seeing the issue, please let me know. -- Caroline Tice cmt...@google.com On Thu, Aug 22, 2013 at 10:07 AM, Caroline Tice cmt...@google.com wrote: I was not aware that the links required Javascript. Can anybody tell me how to fix them so that they don't? -- Caroline Tice cmt...@google.com On Thu, Aug 22, 2013 at 9:58 AM, Joseph S. Myers jos...@codesourcery.com wrote: On Mon, 12 Aug 2013, Caroline Tice wrote: I have finished my first pass at the porting documentation. It is now available on the vtable verification feature web site: http://gcc.gnu.org/wiki/vtv Please provide the links there in a form that does not require JavaScript to read them (in particular, does not require running non-free JavaScript, as per GNU Project principles). That's certainly possible for Google Docs documents; you can read https://docs.google.com/document/pub?id=1ZfyfkB62EFaR4_g4JKm4--guz3vxm9pciOBziMHTnK4pli=1 (GCC Architectural Goals) without JavaScript, for example. -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH/Merge Request] Vtable Verification feature.
On Thu, Aug 8, 2013 at 9:34 AM, Caroline Tice cmt...@google.com wrote: On Thu, Aug 8, 2013 at 3:27 AM, Rainer Orth r...@cebitec.uni-bielefeld.de wrote: Caroline, As an aside, I had a very quick look at libvtv to determine what's required for a port to a non-Linux platform. It would be good if the requirements (currently, ELF, dl_iterate_phdr, __fortify_fail, certainly several more) could be documented to ease porters' task. I will be working on some documentation for this. Thanks. Rainer I have finished my first pass at the porting documentation. It is now available on the vtable verification feature web site: http://gcc.gnu.org/wiki/vtv I've never attempted to write porting documentation before, so I'm hoping that what I wrote is what was needed. If I missed anything important, please let me know. -- Caroline Tice cmt...@google.com
Re: [PATCH/Merge Request] Vtable Verification feature.
Caroline, your libgcc ChangeLog entries are all broken: they lack the initial * as can easily be seen in Emacs' Change Log Mode. Please fix. As an aside, I had a very quick look at libvtv to determine what's required for a port to a non-Linux platform. It would be good if the requirements (currently, ELF, dl_iterate_phdr, __fortify_fail, certainly several more) could be documented to ease porters' task. Thanks. Rainer -- - Rainer Orth, Center for Biotechnology, Bielefeld University
Re: [PATCH/Merge Request] Vtable Verification feature.
Hi Caroline, On 8 Aug 2013, at 11:27, Rainer Orth wrote: As an aside, I had a very quick look at libvtv to determine what's required for a port to a non-Linux platform. Likewise.. It would be good if the requirements (currently, ELF, dl_iterate_phdr, __fortify_fail, certainly several more) could be documented to ease porters' task. +1 Also, in general, it would be very helpful if folks introducing new named sections would use a target hook to fetch them - since section naming/flag conventions are different between (at least) elf and mach-o. thanks Iain
Re: [PATCH/Merge Request] Vtable Verification feature.
On 08/06/13 22:39, Benjamin De Kosnik wrote:
+# Filter out unsupported systems.
+case ${target} in
+ x86_64-*-linux* | i?86-*-linux*)
+ VTV_SUPPORTED=yes
+ ;;
+ powerpc*-*-linux*)
+ ;;
+ sparc*-*-linux*)
+ ;;
+ arm*-*-linux*)
+ ;;
What about powerpc, sparc and arm? Why are they mentioned here if no
actual decision is made about support?
This is more a practical consideration: it's the middle of summer
break. Let's error on the side of caution for the moment, and get
this in causing minimal disruption on a convenient platform that I can
verify myself easily.
Once this is in trunk, let a million flowers bloom! There is no
reason specific platform/target maintainers can't enable it at their
leisure on a per-setup manner and when they can verify testresults
easily.
arm*-*-linux* is broken currently for what looks like the same reasons
as the powerpc port.
I spent some time this morning looking into what it would take to enable
this for arm*-*-linux* today and the first thing that I noticed is that
the testsuite is essentially driven by a shell script that caters to
native testing on the only port that this was developed for. It also
expects -m32 and -m64 to be a standard option in all ports that want to
turn this on. Also because the tests are not using dejagnu it's going to
be a right pain to get this to work for cross-testing and unless that
works reliably one isn't going to be able to get this feature working
easily.
Also whatever is target specific in the testsuite like
environment-fail-32.s and environment-fail-64.s needs to live in it's
own target folders. Basically this feature needs to follow conventions
that exist already in other parts of the source base or atleast have
plans to get there surely ?
I haven't seen this in the reviews that I've seen so far, so apologies
if this has already been raised.
regards
Ramana
Re: [PATCH/Merge Request] Vtable Verification feature.
On Thu, 8 Aug 2013, Ramana Radhakrishnan wrote: I spent some time this morning looking into what it would take to enable this for arm*-*-linux* today and the first thing that I noticed is that the testsuite is essentially driven by a shell script that caters to native testing on the only port that this was developed for. It also expects -m32 and -m64 to be a standard option in all ports that want to turn this on. Also because the tests are not using dejagnu it's going to be a right pain to get this to work for cross-testing and unless that works reliably one isn't going to be able to get this feature working easily. Clearly it's got to be converted to DejaGnu (and must avoid depending on a build directory at all, installed testing with runtest --tool libvtv, having created an appropriate site.exp, should work as well). It's looking rather like this whole feature hasn't been adequately reviewed before commit -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH/Merge Request] Vtable Verification feature.
On Thu, Aug 8, 2013 at 5:55 AM, Ramana Radhakrishnan ramra...@arm.com wrote:
On 08/06/13 22:39, Benjamin De Kosnik wrote:
+# Filter out unsupported systems.
+case ${target} in
+ x86_64-*-linux* | i?86-*-linux*)
+ VTV_SUPPORTED=yes
+ ;;
+ powerpc*-*-linux*)
+ ;;
+ sparc*-*-linux*)
+ ;;
+ arm*-*-linux*)
+ ;;
What about powerpc, sparc and arm? Why are they mentioned here if no
actual decision is made about support?
This is more a practical consideration: it's the middle of summer
break. Let's error on the side of caution for the moment, and get
this in causing minimal disruption on a convenient platform that I can
verify myself easily.
Once this is in trunk, let a million flowers bloom! There is no
reason specific platform/target maintainers can't enable it at their
leisure on a per-setup manner and when they can verify testresults
easily.
arm*-*-linux* is broken currently for what looks like the same reasons as
the powerpc port.
Have you tried Benjamin Kosnik's patch? Does it fix the problem?
I spent some time this morning looking into what it would take to enable
this for arm*-*-linux* today and the first thing that I noticed is that the
testsuite is essentially driven by a shell script that caters to native
testing on the only port that this was developed for. It also expects -m32
and -m64 to be a standard option in all ports that want to turn this on.
Also because the tests are not using dejagnu it's going to be a right pain
to get this to work for cross-testing and unless that works reliably one
isn't going to be able to get this feature working easily.
Also whatever is target specific in the testsuite like environment-fail-32.s
and environment-fail-64.s needs to live in it's own target folders.
Basically this feature needs to follow conventions that exist already in
other parts of the source base or atleast have plans to get there surely ?
I haven't seen this in the reviews that I've seen so far, so apologies if
this has already been raised.
I know that the testsuite, as committed, is not in DejaGnu format and
that it will need to be converted to DejaGnu format. I also know that
it is currently specific to the platform on which the feature was
developed. The plan has always been to fix this. There was some
private discussion about whether it would be better to commit the
testsuite in its current state or not to commit a testsuite at all
with the initial project commit. I was told that it would be better
to show that there is some way of testing this feature, even if the
testsuite is not in its final form, than to not show any way of
testing it at all.
I will work on getting the testsuite into a better form and also on
writing up some documentation on the platform-specific pieces of the
vtable verification feature (for those who wish to port it to other
platforms).
Re: [PATCH/Merge Request] Vtable Verification feature.
On Thu, Aug 8, 2013 at 3:27 AM, Rainer Orth r...@cebitec.uni-bielefeld.de wrote: Caroline, your libgcc ChangeLog entries are all broken: they lack the initial * as can easily be seen in Emacs' Change Log Mode. Please fix. Wow, I'm really sorry about that! I don't know how that happened. I'll fix it immediately. As an aside, I had a very quick look at libvtv to determine what's required for a port to a non-Linux platform. It would be good if the requirements (currently, ELF, dl_iterate_phdr, __fortify_fail, certainly several more) could be documented to ease porters' task. I will be working on some documentation for this. Thanks. Rainer -- - Rainer Orth, Center for Biotechnology, Bielefeld University
Re: [PATCH/Merge Request] Vtable Verification feature.
arm*-*-linux* is broken currently for what looks like the same reasons as
the powerpc port.
Have you tried Benjamin Kosnik's patch? Does it fix the problem?
I hadn't but that seems to have fixed the issue. Thanks.
I spent some time this morning looking into what it would take to enable
this for arm*-*-linux* today and the first thing that I noticed is that the
testsuite is essentially driven by a shell script that caters to native
testing on the only port that this was developed for. It also expects -m32
and -m64 to be a standard option in all ports that want to turn this on.
Also because the tests are not using dejagnu it's going to be a right pain
to get this to work for cross-testing and unless that works reliably one
isn't going to be able to get this feature working easily.
Also whatever is target specific in the testsuite like environment-fail-32.s
and environment-fail-64.s needs to live in it's own target folders.
Basically this feature needs to follow conventions that exist already in
other parts of the source base or atleast have plans to get there surely ?
I haven't seen this in the reviews that I've seen so far, so apologies if
this has already been raised.
I know that the testsuite, as committed, is not in DejaGnu format and
that it will need to be converted to DejaGnu format. I also know that
it is currently specific to the platform on which the feature was
developed. The plan has always been to fix this. There was some
private discussion about whether it would be better to commit the
testsuite in its current state or not to commit a testsuite at all
with the initial project commit. I was told that it would be better
to show that there is some way of testing this feature, even if the
testsuite is not in its final form, than to not show any way of
testing it at all.
Good to know there is atleast a plan to fix this. It would have been
better to state this in a public review or as part of your merge request
or indeed fix this properly before merging .
I will work on getting the testsuite into a better form and also on
writing up some documentation on the platform-specific pieces of the
vtable verification feature (for those who wish to port it to other
platforms).
Cool - thanks .
So I tried playing with it on native arm*-*-*linux builds for sometime
and ran into the first problem which came while just building vtv_start.c .
/bin/bash ./libtool --tag=CC --mode=compile
/home/ramrad01/build-vtv1/./gcc/xgcc -B/home/ramrad01/build-vtv1/./gcc/
-B/usr/local/armv7l-unknown-linux-gnueabihf/bin/
-B/usr/local/armv7l-unknown-linux-gnueabihf/lib/ -isystem
/usr/local/armv7l-unknown-linux-gnueabihf/include -isystem
/usr/local/armv7l-unknown-linux-gnueabihf/sys-include -I.
-I../../../gcc/libvtv -I../../../gcc/libvtv/../include -D_GNU_SOURCE
-Wall -Wextra -fno-exceptions -g -O2 -MT vtv_start.lo -MD -MP -MF
.deps/vtv_start.Tpo -c -o vtv_start.lo vtv_start.c
libtool: compile: /home/ramrad01/build-vtv1/./gcc/xgcc
-B/home/ramrad01/build-vtv1/./gcc/
-B/usr/local/armv7l-unknown-linux-gnueabihf/bin/
-B/usr/local/armv7l-unknown-linux-gnueabihf/lib/ -isystem
/usr/local/armv7l-unknown-linux-gnueabihf/include -isystem
/usr/local/armv7l-unknown-linux-gnueabihf/sys-include -I.
-I../../../gcc/libvtv -I../../../gcc/libvtv/../include -D_GNU_SOURCE
-Wall -Wextra -fno-exceptions -g -O2 -MT vtv_start.lo -MD -MP -MF
.deps/vtv_start.Tpo -c vtv_start.c -fPIC -DPIC -o .libs/vtv_start.o
vtv_start.c:57:1: warning: constructor priorities from 0 to 100 are
reserved for the implementation [enabled by default]
{
^
vtv_start.c:65:3: internal compiler error: Segmentation fault
= { };
^
0x3aa3f5 crash_signal
../../gcc/gcc/toplev.c:335
0x52a669 default_elf_asm_named_section(char const*, unsigned int,
tree_node*)
../../gcc/gcc/varasm.c:6219
0x52c041 switch_to_section
../../gcc/gcc/varasm.c:7035
0x52c5b1 output_object_block
../../gcc/gcc/varasm.c:7209
0x52c5b1 output_object_block_htab
../../gcc/gcc/varasm.c:7270
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See http://gcc.gnu.org/bugs.html for instructions.
make[2]: *** [vtv_start.lo] Error 1
make[2]: Leaving directory
`/home/ramrad01/build-vtv1/armv7l-unknown-linux-gnueabihf/libvtv'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory
`/home/ramrad01/build-vtv1/armv7l-unknown-linux-gnueabihf/libvtv'
make: *** [all] Error 2
It might be because of inconsistencies for handling comdat linkage with
regards to the ARM C++ ABI variations but it's too late for me to dig
further tonight.
regards
Ramana
Re: [PATCH/Merge Request] Vtable Verification feature.
On Wed, Aug 7, 2013 at 6:03 PM, Joseph S. Myers jos...@codesourcery.com wrote: Looking at the patch as committed, there seems to be some confusion about the nature of the --enable-vtable-verify configure option. Yes, there is a bit. It's documented only for libstdc++. Now, I still consider the existence of separate documentation for libstdc++ configure options to be unfortunate - I think all configure options for GCC should be documented in one place - but that's a separate matter. Although only documented for libstdc++, it actually appears to be used in the libgcc and libvtv configure scripts. The original intent of this flag was that if --enable-vtable-verify was NOT used, then NOTHING having to do with vtable verification would be built anywhere in the compiler, i.e. the binaries, libraries, etc. would not contain anything that wasn't there before the vtable verification patch was committed. The idea (and implementation) has evolved a bit, and I am not sure what the right way to handle this option is at the moment. Given that it has effects on more than just libstdc++, it needs documenting in gcc/doc/install.texi, the main documentation of configure options for GCC. I recently submitted a patch that adds the documentation for my current understanding of the way this option works. Then there's the question of what the semantics should be. My presumption is that the feature should be available for all GCC builds, at least by default on platforms where it can work, and the only thing needing a configure option should be whether the checks are enabled for libstdc++ itself (and ideally that would work by multilibbing libstdc++ rather than needing separate GCC builds to get libstdc++ with and without the checks). Actually if you ever want to use the feature with your compiler you should build your compiler with --enable-vtable-verify. This will, as you noted, insert calls in libstdc++ to build the verification data structures and to verify the virtual calls in libstdc++ itself. However libstdc++ itself contains 'stub' (do-nothing) versions of the functions that build the data structures and perform the verification. So if you want to turn on verification with libstdc++, you link it with libvtv (which contains the real versions of those functions) and if you don't want verification with libstdc++ you just don't link in libvtv. There is no need to multiple versions of libstdc++. Thus if the platform supports the feature, all relevant libgcc files should be built, and anything for libstdc++ needed for user code to use the feature should be built - the only thing not enabled by default would be checks for libstdc++'s classes' own vtables. (And unless there are difficulties in building the libgcc files on some systems, they could be built unconditionally, whether or not any other support needed for libvtv is present. I supposed the libgcc files could be built all the time (on systems that support libvtv). Would there be any down side to this? Actually, it looks like they may depend on an ELF target, but not on anything more.) Could you confirm that the libstdc++ ABI is not affected by the configure option - that the same symbols, at the same symbol versions, with the same semantics, are exported from libstdc++.so for builds with and without the feature enabled? The libstdc++ ABI has been enhanced to export the vtable verification functions for which it contains stub versions (otherwise they could never be overwritten by the versions in libvtv). Other than that the libstdc++ ABI exports the same symbols at the same symbol versions with the same semantics. I believe this export is unconditional. The file cp/vtable-class-hierarchy.c includes tm.h. Includes of tm.h from front ends are discouraged, and should have comments on them listing what target macros are used by the file in question (and so need to be converted to hooks before the include can be removed). Could you add such a comment to the #include (or if it's redundant, remove all redundant #includes from that file)? You have a +#define MAX_SET_SIZE 5000 which superficially looks like an arbitrary limit. Could you add a comment explaining why no input files, not matter how extreme, could ever exceed the limit of 5000? You have a couple of gcc_asserts regarding this limit, and an on-stack array for which it's at least not immediately obvious that all accesses are checked to ensure that a buffer overrun is impossible. If you have an arbitrary limit, and some input *can* exceed it (so triggering the gcc_asserts or buffer overrun), the right fix is of course to remove it, probably using vec.h to produce a dynamically growing array instead of hardcoding a size at all. But failing that, exceeding the limit must result in a sorry () call, not an assertion failure or buffer overrun, neither of which is acceptable behavior for any compiler input whatever. Other people have
Re: [PATCH/Merge Request] Vtable Verification feature.
On Thu, 8 Aug 2013, Caroline Tice wrote: Actually if you ever want to use the feature with your compiler you should build your compiler with --enable-vtable-verify. This will, as you noted, insert calls in libstdc++ to build the verification data structures and to verify the virtual calls in libstdc++ itself. However libstdc++ itself contains 'stub' (do-nothing) versions of the functions that build the data structures and perform the verification. So if you want to turn on verification with libstdc++, you link it with libvtv (which contains the real versions of those functions) and if you don't want verification with libstdc++ you just don't link in libvtv. There is no need to multiple versions of libstdc++. But presumably libstdc++ with the extra calls is less efficient than libstdc++ without them, even if the calls are just to stub functions? A GNU/Linux distributor would likely want to enable their users to use this functionality, meaning distributing a GCC build with libvtv included and a version of libstdc++ with the calls present for users who wish to build programs (linking with libstdc++ and libvtv) for which the calls in libstdc++ are checked. But they might not want to have even the stub calls executed for all installed C++ programs. So it would seem natural to provide both copies of libstdc++ - and desirable for this to be possible without needing separate GCC builds. I supposed the libgcc files could be built all the time (on systems that support libvtv). Would there be any down side to this? In my model, it's appropriate to build those independent of the libstdc++ configure option - and likewise to build everything in libvtv independent of the configure option. There should be no need for the configure option to affect anything other than, at most, libstdc++ - that is, the handling of the option in other configure scripts should be removed, with the case where the relevant support is built being enabled by default. For certain use cases the current working directory is not our preferred place to put the log files. I have recently submitted a patch that tries to use environment variables to determine where to put the log files. It first checks to see if the user has defined VTV_LOGS_DIR, in which case it will use that. If that fails, it tries to find and use HOME. If that also fails, it falls back on using the working directory. I hope that is ok? I have modified the call to open to take O_NOFOLLOW, but O_EXCL will do the wrong thing, as we sometimes wish to append to the log files and O_EXCL fails if you attempt to open an existing file (according to the documentation I read). The list of directories seems plausible (better than using /tmp, anyway). Classically O_EXCL was needed in such cases (if the log might end up getting written to a file in a directory also writable by an attacker) not just because of symlink attacks but also because it was possible to create a hard link to another user's file, with similar potential for attacks as with symlinks. Nowadays systems are more likely to restrict such hard link creation (but they are also likely to have similar restrictions to make symlink attacks harder as well). -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH/Merge Request] Vtable Verification feature.
On 08/08/2013 06:34 PM, Caroline Tice wrote: Actually if you ever want to use the feature with your compiler you should build your compiler with --enable-vtable-verify. This will, as you noted, insert calls in libstdc++ to build the verification data structures and to verify the virtual calls in libstdc++ itself. However libstdc++ itself contains 'stub' (do-nothing) versions of the functions that build the data structures and perform the verification. How do you ensure that the libvtv versions get used rather than the stubs in libstdc++? Jason
Re: [PATCH/Merge Request] Vtable Verification feature.
Which version gets used depends on their ordering in the link line. When -fvtable-verify=std or -fvtable-verify=preinit is used, the gcc driver inserts -lvtv very early into the link line (earlier than -lstdc++), so this happens automatically. -- Caroline cmt...@google.com On Thu, Aug 8, 2013 at 4:33 PM, Jason Merrill ja...@redhat.com wrote: On 08/08/2013 06:34 PM, Caroline Tice wrote: Actually if you ever want to use the feature with your compiler you should build your compiler with --enable-vtable-verify. This will, as you noted, insert calls in libstdc++ to build the verification data structures and to verify the virtual calls in libstdc++ itself. However libstdc++ itself contains 'stub' (do-nothing) versions of the functions that build the data structures and perform the verification. How do you ensure that the libvtv versions get used rather than the stubs in libstdc++? Jason
Re: [PATCH/Merge Request] Vtable Verification feature.
Revision 201555 breaks boostrap on x86_64-apple-darwin10: ... Checking multilib configuration for libvtv... make all-recursive Making all in testsuite /bin/sh: line 0: cd: testsuite: No such file or directory make[4]: *** [all-recursive] Error 1 make[3]: *** [all] Error 2 make[2]: *** [all-stage1-target-libvtv] Error 2 make[1]: *** [stage1-bubble] Error 2 make: *** [all] Error 2 According to * configure.ac: Add target-libvtv to target_libraries; disable libvtv on non-linux systems; add target-libvtv to noconfigdirs; add libsupc++/.libs to C++ library search paths. libvtv should not be built on darwin, but in config.log I see ... configure:3222: checking for libvtv support configure:3232: result: yes ... TIA Dominique
Re: [PATCH/Merge Request] Vtable Verification feature.
libvtv was supposed to be automatically disabled for darwin; we are in the process of trying to figure out why this did not work as it was supposed to. In the meantime, if you add --disable-libvtv explicitly to your configure command, that should turn off the attempts to configure/build it. I'll get another patch out to fix the problem as soon as I can. -- Caroline Tice cmt...@google.com On Wed, Aug 7, 2013 at 5:29 AM, Dominique Dhumieres domi...@lps.ens.fr wrote: Revision 201555 breaks boostrap on x86_64-apple-darwin10: ... Checking multilib configuration for libvtv... make all-recursive Making all in testsuite /bin/sh: line 0: cd: testsuite: No such file or directory make[4]: *** [all-recursive] Error 1 make[3]: *** [all] Error 2 make[2]: *** [all-stage1-target-libvtv] Error 2 make[1]: *** [stage1-bubble] Error 2 make: *** [all] Error 2 According to * configure.ac: Add target-libvtv to target_libraries; disable libvtv on non-linux systems; add target-libvtv to noconfigdirs; add libsupc++/.libs to C++ library search paths. libvtv should not be built on darwin, but in config.log I see ... configure:3222: checking for libvtv support configure:3232: result: yes ... TIA Dominique
Re: [PATCH/Merge Request] Vtable Verification feature.
hi Caroline, A (very) quick look suggests that only *) results in a return of UNSUPPORTED from libvtv/configure.tgt Do you know if anyone is working on a Darwin port for this lib? thanks, Iain On 7 Aug 2013, at 17:57, Caroline Tice wrote: libvtv was supposed to be automatically disabled for darwin; we are in the process of trying to figure out why this did not work as it was supposed to. In the meantime, if you add --disable-libvtv explicitly to your configure command, that should turn off the attempts to configure/build it. I'll get another patch out to fix the problem as soon as I can. -- Caroline Tice cmt...@google.com On Wed, Aug 7, 2013 at 5:29 AM, Dominique Dhumieres domi...@lps.ens.fr wrote: Revision 201555 breaks boostrap on x86_64-apple-darwin10: ... Checking multilib configuration for libvtv... make all-recursive Making all in testsuite /bin/sh: line 0: cd: testsuite: No such file or directory make[4]: *** [all-recursive] Error 1 make[3]: *** [all] Error 2 make[2]: *** [all-stage1-target-libvtv] Error 2 make[1]: *** [stage1-bubble] Error 2 make: *** [all] Error 2 According to * configure.ac: Add target-libvtv to target_libraries; disable libvtv on non-linux systems; add target-libvtv to noconfigdirs; add libsupc++/.libs to C++ library search paths. libvtv should not be built on darwin, but in config.log I see ... configure:3222: checking for libvtv support configure:3232: result: yes ... TIA Dominique
Re: [PATCH/Merge Request] Vtable Verification feature.
Hi Iain, Thanks for the pointer (I had noticed this but I appreciate the help!). I do not know of anyone working on a Darwin port for this at this time. -- Caroline Tice cmt...@google.com On Wed, Aug 7, 2013 at 10:31 AM, Iain Sandoe i...@codesourcery.com wrote: hi Caroline, A (very) quick look suggests that only *) results in a return of UNSUPPORTED from libvtv/configure.tgt Do you know if anyone is working on a Darwin port for this lib? thanks, Iain On 7 Aug 2013, at 17:57, Caroline Tice wrote: libvtv was supposed to be automatically disabled for darwin; we are in the process of trying to figure out why this did not work as it was supposed to. In the meantime, if you add --disable-libvtv explicitly to your configure command, that should turn off the attempts to configure/build it. I'll get another patch out to fix the problem as soon as I can. -- Caroline Tice cmt...@google.com On Wed, Aug 7, 2013 at 5:29 AM, Dominique Dhumieres domi...@lps.ens.fr wrote: Revision 201555 breaks boostrap on x86_64-apple-darwin10: ... Checking multilib configuration for libvtv... make all-recursive Making all in testsuite /bin/sh: line 0: cd: testsuite: No such file or directory make[4]: *** [all-recursive] Error 1 make[3]: *** [all] Error 2 make[2]: *** [all-stage1-target-libvtv] Error 2 make[1]: *** [stage1-bubble] Error 2 make: *** [all] Error 2 According to * configure.ac: Add target-libvtv to target_libraries; disable libvtv on non-linux systems; add target-libvtv to noconfigdirs; add libsupc++/.libs to C++ library search paths. libvtv should not be built on darwin, but in config.log I see ... configure:3222: checking for libvtv support configure:3232: result: yes ... TIA Dominique
Re: [PATCH/Merge Request] Vtable Verification feature.
Caroline, When libvtv was checked in, libvtv/autom4te.cache subdirectory was committed as well. I don't think that this was intended. - David
Re: [PATCH/Merge Request] Vtable Verification feature.
+ gcc_patches list On Wed, Aug 7, 2013 at 2:56 PM, Caroline Tice cmt...@google.com wrote: No, it was not intended. How do I undo that? -- Caroline cmt...@google.com On Wed, Aug 7, 2013 at 2:54 PM, David Edelsohn dje@gmail.com wrote: Caroline, When libvtv was checked in, libvtv/autom4te.cache subdirectory was committed as well. I don't think that this was intended. - David
Re: [PATCH/Merge Request] Vtable Verification feature.
.. I removed it. Thanks, Paolo.
Re: [PATCH/Merge Request] Vtable Verification feature.
Thank you! -- Caroline On Wed, Aug 7, 2013 at 3:09 PM, Paolo Carlini paolo.carl...@oracle.com wrote: .. I removed it. Thanks, Paolo.
Re: [PATCH/Merge Request] Vtable Verification feature.
On Wed, Aug 07, 2013 at 02:29:35PM +0200, Dominique Dhumieres wrote:
Revision 201555 breaks boostrap on x86_64-apple-darwin10:
...
Checking multilib configuration for libvtv...
make all-recursive
Making all in testsuite
/bin/sh: line 0: cd: testsuite: No such file or directory
make[4]: *** [all-recursive] Error 1
make[3]: *** [all] Error 2
make[2]: *** [all-stage1-target-libvtv] Error 2
make[1]: *** [stage1-bubble] Error 2
make: *** [all] Error 2
According to
* configure.ac: Add target-libvtv to target_libraries; disable libvtv
on non-linux systems; add target-libvtv to noconfigdirs; add
libsupc++/.libs to C++ library search paths.
libvtv should not be built on darwin, but in config.log I see
...
configure:3222: checking for libvtv support
configure:3232: result: yes
...
Same on powerpc*-linux.
In looking at it briefly, I see several things wrong with the way libvtv is
configured. Here is the guts of libvtv/configure.tgt:
case ${target} in
x86_64-*-linux* | i?86-*-linux*)
VTV_SUPPORTED=yes
;;
powerpc*-*-linux*)
;;
sparc*-*-linux*)
;;
arm*-*-linux*)
;;
x86_64-*-darwin[1]* | i?86-*-darwin[1]*)
VTV_SUPPORTED=no
;;
*)
UNSUPPORTED=1
;;
esac
In the two places that use this script (top level configure, and configure
within libvtv), they both run the .tgt script with . ./file,. However, they
do no explicitly clear the UNSUPPORTED variable. So perhaps you might have
some other script within the configure process use UNSUPPORTED, and it would
not get built for the system that supports it.
Then there is the problem that the two callers use different shell variables to
check whether the library is supported or not. I tend to think the top level
configure should be changed to look at VTV_SUPPORTED and not UNSUPPORTED, and
that only the systems that it is know to work on should set the variable to
yes.
I.e.
case ${target} in
x86_64-*-linux* | i?86-*-linux*)
VTV_SUPPORTED=yes
unset UNSUPPORTED
;;
*)
VTV_SUPPORTED=no
UNSUPPORTED=1
;;
esac
(remove the UNSUPPORTED lines if we change the top level configure).
--
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460, USA
email: meiss...@linux.vnet.ibm.com, phone: +1 (978) 899-4797
Re: [PATCH/Merge Request] Vtable Verification feature.
Looking at the patch as committed, there seems to be some confusion about the nature of the --enable-vtable-verify configure option. It's documented only for libstdc++. Now, I still consider the existence of separate documentation for libstdc++ configure options to be unfortunate - I think all configure options for GCC should be documented in one place - but that's a separate matter. Although only documented for libstdc++, it actually appears to be used in the libgcc and libvtv configure scripts. Given that it has effects on more than just libstdc++, it needs documenting in gcc/doc/install.texi, the main documentation of configure options for GCC. Then there's the question of what the semantics should be. My presumption is that the feature should be available for all GCC builds, at least by default on platforms where it can work, and the only thing needing a configure option should be whether the checks are enabled for libstdc++ itself (and ideally that would work by multilibbing libstdc++ rather than needing separate GCC builds to get libstdc++ with and without the checks). Thus if the platform supports the feature, all relevant libgcc files should be built, and anything for libstdc++ needed for user code to use the feature should be built - the only thing not enabled by default would be checks for libstdc++'s classes' own vtables. (And unless there are difficulties in building the libgcc files on some systems, they could be built unconditionally, whether or not any other support needed for libvtv is present. Actually, it looks like they may depend on an ELF target, but not on anything more.) Could you confirm that the libstdc++ ABI is not affected by the configure option - that the same symbols, at the same symbol versions, with the same semantics, are exported from libstdc++.so for builds with and without the feature enabled? The file cp/vtable-class-hierarchy.c includes tm.h. Includes of tm.h from front ends are discouraged, and should have comments on them listing what target macros are used by the file in question (and so need to be converted to hooks before the include can be removed). Could you add such a comment to the #include (or if it's redundant, remove all redundant #includes from that file)? You have a +#define MAX_SET_SIZE 5000 which superficially looks like an arbitrary limit. Could you add a comment explaining why no input files, not matter how extreme, could ever exceed the limit of 5000? You have a couple of gcc_asserts regarding this limit, and an on-stack array for which it's at least not immediately obvious that all accesses are checked to ensure that a buffer overrun is impossible. If you have an arbitrary limit, and some input *can* exceed it (so triggering the gcc_asserts or buffer overrun), the right fix is of course to remove it, probably using vec.h to produce a dynamically growing array instead of hardcoding a size at all. But failing that, exceeding the limit must result in a sorry () call, not an assertion failure or buffer overrun, neither of which is acceptable behavior for any compiler input whatever. Other people have previously commented about the logs *generated by the compiler*. The issue raised regarding the directories for those logs has been fixed so my only comment there is that the diagnostics for failure to open those files have several problems: * Diagnostics should not start with a capital letter. * Use % and % as quotes in diagnostics. * Use %m in the diagnostic to print the error text corresponding to the errno value from the failure to open. * I doubt input_location is particularly meaningful as a location for these diagnostics; warning_at with UNKNOWN_LOCATION may be better. But there's a much more serious issue with logs generated *by libvtv*. These appear to use fixed filenames in a fixed directory /tmp/vtv_logs. That's always a mistake (I've encountered such trouble with QEMU using such a fixed log name in the past). Not only that, neither O_EXCL nor O_NOFOLLOW is used so you have an obvious security hole. Using such global locations is simply never OK; you need to arrange for such failures to go somewhere that will never conflict with other users. You can't create a directory in /tmp (because proper ownership and permissions for such a shared directory would be the same as for /tmp itself, rather than leaving it owned by the first user to create it, and an unprivileged process can't ensure that). I suggest: * Prefer the current working directory (as used for core dumps) over /tmp (but if /tmp is used as a fallback, it needs to be /tmp not a subdirectory thereof). * Name the generated file with a name that includes the program name, its pid, and a random string, to reduce the chance of conflicts. * In any case, always open with O_NOFOLLOW (if defined) and O_EXCL to avoid symlink attacks. -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH/Merge Request] Vtable Verification feature.
On Tue, Aug 6, 2013 at 10:35 AM, Caroline Tice cmt...@google.com wrote: choose_tmpdir(), from libiberty is not mentioned in any .h file, so cannot (at the moment) be used here. Is it OK for me to add choose_tmpdir to libiberty.h? Well, maybe. But the approach looks a bit odd to me. Also very underdocumented--the -fvtv-counts option needs expanding. And I don't see why a user would ever invoke it anyhow, so perhaps it should be a --param rather than a -f option. The output to the file doesn't have any indication of what file is being compiled, so it will be ambiguous when run in parallel. Why shouldn't this data be dumped into a standard GCC dump file as we do with most data like this? Your sum-vtv-counts program could still work, it would just take a set of dump files and look for the specific strings that it cares about. Ian
Re: [PATCH/Merge Request] Vtable Verification feature.
On Tue, Aug 6, 2013 at 11:43 AM, Caroline Tice cmt...@google.com wrote: On Tue, Aug 6, 2013 at 11:31 AM, Ian Lance Taylor i...@google.com wrote: The output to the file doesn't have any indication of what file is being compiled, so it will be ambiguous when run in parallel. You are mistaken. It outputs one line to the log file for each compilation unit. The output line begins with the name of the file that was being compiled. In my use case, I have used this to build a very large system, which resulted in something like at 8000 line log file of counts, which I then used my sum script on to see how the verifications were going. I was mistaken in detail but I'm not sure I was mistaken in principle. What happens if you are building the large system twice in different directories on the same machine? Or, for that matter, if two different people are doing so? Or if one person did it a while ago, and now you want to do it, but you can't open the file because it is owned by the other person? Maybe you should simply change -fvtv-counts to take a file name, then we don't have to worry about any of this. Ian
Re: [PATCH/Merge Request] Vtable Verification feature.
On Tue, Aug 6, 2013 at 12:07 PM, Caroline Tice cmt...@google.com wrote: On Tue, Aug 6, 2013 at 12:02 PM, Ian Lance Taylor i...@google.com wrote: On Tue, Aug 6, 2013 at 11:43 AM, Caroline Tice cmt...@google.com wrote: On Tue, Aug 6, 2013 at 11:31 AM, Ian Lance Taylor i...@google.com wrote: The output to the file doesn't have any indication of what file is being compiled, so it will be ambiguous when run in parallel. You are mistaken. It outputs one line to the log file for each compilation unit. The output line begins with the name of the file that was being compiled. In my use case, I have used this to build a very large system, which resulted in something like at 8000 line log file of counts, which I then used my sum script on to see how the verifications were going. I was mistaken in detail but I'm not sure I was mistaken in principle. What happens if you are building the large system twice in different directories on the same machine? Or, for that matter, if two different people are doing so? Or if one person did it a while ago, and now you want to do it, but you can't open the file because it is owned by the other person? Maybe you should simply change -fvtv-counts to take a file name, then we don't have to worry about any of this. It's not quite that simple: the -fvtv-counts flag actually causes two files to be created; also there's another flag, -fvtv-debug that generates a third file (i wanted a lot of information when I was working on and debugging this feature). Also if users are arbitrarily allowed to name the counts file anything, the summing script program I wrote won't be able to find them. That doesn't seem like a compelling argument to me, since one could pass the file names to the summing script as well. As far as I can see, on a multi-user system, there is no reasonable alternative to permitting the user to specify the file names to use, or at least a directory where the files should be placed. And if permit that, why not simply require it? Ian
Re: [PATCH/Merge Request] Vtable Verification feature.
I was talking with Diego, and he suggested the possibility of putting the log files in the same directory that the gcc dump files go, i.e. the one specified by dump_base_name. Do you think that would be acceptable? -- Caroline Tice cmt...@google.com On Tue, Aug 6, 2013 at 12:12 PM, Ian Lance Taylor i...@google.com wrote: On Tue, Aug 6, 2013 at 11:43 AM, Caroline Tice cmt...@google.com wrote: On Tue, Aug 6, 2013 at 11:31 AM, Ian Lance Taylor i...@google.com wrote: The output to the file doesn't have any indication of what file is being compiled, so it will be ambiguous when run in parallel. You are mistaken. It outputs one line to the log file for each compilation unit. The output line begins with the name of the file that was being compiled. In my use case, I have used this to build a very large system, which resulted in something like at 8000 line log file of counts, which I then used my sum script on to see how the verifications were going. I was mistaken in detail but I'm not sure I was mistaken in principle. What happens if you are building the large system twice in different directories on the same machine? Or, for that matter, if two different people are doing so? Or if one person did it a while ago, and now you want to do it, but you can't open the file because it is owned by the other person? Maybe you should simply change -fvtv-counts to take a file name, then we don't have to worry about any of this. It's not quite that simple: the -fvtv-counts flag actually causes two files to be created; also there's another flag, -fvtv-debug that generates a third file (i wanted a lot of information when I was working on and debugging this feature). Also if users are arbitrarily allowed to name the counts file anything, the summing script program I wrote won't be able to find them. That doesn't seem like a compelling argument to me, since one could pass the file names to the summing script as well. As far as I can see, on a multi-user system, there is no reasonable alternative to permitting the user to specify the file names to use, or at least a directory where the files should be placed. And if permit that, why not simply require it? Ian
Re: [PATCH/Merge Request] Vtable Verification feature.
Actually, I think that was dump_dir_name. -- Caroline On Tue, Aug 6, 2013 at 1:12 PM, Caroline Tice cmt...@google.com wrote: I was talking with Diego, and he suggested the possibility of putting the log files in the same directory that the gcc dump files go, i.e. the one specified by dump_base_name. Do you think that would be acceptable? -- Caroline Tice cmt...@google.com On Tue, Aug 6, 2013 at 12:12 PM, Ian Lance Taylor i...@google.com wrote: On Tue, Aug 6, 2013 at 11:43 AM, Caroline Tice cmt...@google.com wrote: On Tue, Aug 6, 2013 at 11:31 AM, Ian Lance Taylor i...@google.com wrote: The output to the file doesn't have any indication of what file is being compiled, so it will be ambiguous when run in parallel. You are mistaken. It outputs one line to the log file for each compilation unit. The output line begins with the name of the file that was being compiled. In my use case, I have used this to build a very large system, which resulted in something like at 8000 line log file of counts, which I then used my sum script on to see how the verifications were going. I was mistaken in detail but I'm not sure I was mistaken in principle. What happens if you are building the large system twice in different directories on the same machine? Or, for that matter, if two different people are doing so? Or if one person did it a while ago, and now you want to do it, but you can't open the file because it is owned by the other person? Maybe you should simply change -fvtv-counts to take a file name, then we don't have to worry about any of this. It's not quite that simple: the -fvtv-counts flag actually causes two files to be created; also there's another flag, -fvtv-debug that generates a third file (i wanted a lot of information when I was working on and debugging this feature). Also if users are arbitrarily allowed to name the counts file anything, the summing script program I wrote won't be able to find them. That doesn't seem like a compelling argument to me, since one could pass the file names to the summing script as well. As far as I can see, on a multi-user system, there is no reasonable alternative to permitting the user to specify the file names to use, or at least a directory where the files should be placed. And if permit that, why not simply require it? Ian
Re: [PATCH/Merge Request] Vtable Verification feature.
On Tue, Aug 6, 2013 at 1:12 PM, Caroline Tice cmt...@google.com wrote: I was talking with Diego, and he suggested the possibility of putting the log files in the same directory that the gcc dump files go, i.e. the one specified by dump_base_name. Do you think that would be acceptable? Sure. Ian On Tue, Aug 6, 2013 at 12:12 PM, Ian Lance Taylor i...@google.com wrote: On Tue, Aug 6, 2013 at 11:43 AM, Caroline Tice cmt...@google.com wrote: On Tue, Aug 6, 2013 at 11:31 AM, Ian Lance Taylor i...@google.com wrote: The output to the file doesn't have any indication of what file is being compiled, so it will be ambiguous when run in parallel. You are mistaken. It outputs one line to the log file for each compilation unit. The output line begins with the name of the file that was being compiled. In my use case, I have used this to build a very large system, which resulted in something like at 8000 line log file of counts, which I then used my sum script on to see how the verifications were going. I was mistaken in detail but I'm not sure I was mistaken in principle. What happens if you are building the large system twice in different directories on the same machine? Or, for that matter, if two different people are doing so? Or if one person did it a while ago, and now you want to do it, but you can't open the file because it is owned by the other person? Maybe you should simply change -fvtv-counts to take a file name, then we don't have to worry about any of this. It's not quite that simple: the -fvtv-counts flag actually causes two files to be created; also there's another flag, -fvtv-debug that generates a third file (i wanted a lot of information when I was working on and debugging this feature). Also if users are arbitrarily allowed to name the counts file anything, the summing script program I wrote won't be able to find them. That doesn't seem like a compelling argument to me, since one could pass the file names to the summing script as well. As far as I can see, on a multi-user system, there is no reasonable alternative to permitting the user to specify the file names to use, or at least a directory where the files should be placed. And if permit that, why not simply require it? Ian
Re: [PATCH/Merge Request] Vtable Verification feature.
+# Filter out unsupported systems.
+case ${target} in
+ x86_64-*-linux* | i?86-*-linux*)
+ VTV_SUPPORTED=yes
+ ;;
+ powerpc*-*-linux*)
+ ;;
+ sparc*-*-linux*)
+ ;;
+ arm*-*-linux*)
+ ;;
What about powerpc, sparc and arm? Why are they mentioned here if no
actual decision is made about support?
This is more a practical consideration: it's the middle of summer
break. Let's error on the side of caution for the moment, and get
this in causing minimal disruption on a convenient platform that I can
verify myself easily.
On a practical note, the libsanitizer acceptance
criteria was/is as above, seems sensible to do the same thing with
libvtv.
Once this is in trunk, let a million flowers bloom! There is no
reason specific platform/target maintainers can't enable it at their
leisure on a per-setup manner and when they can verify testresults
easily.
-benjamin
Re: [PATCH/Merge Request] Vtable Verification feature.
On Tue, Aug 6, 2013 at 2:39 PM, Benjamin De Kosnik b...@redhat.com wrote:
+# Filter out unsupported systems.
+case ${target} in
+ x86_64-*-linux* | i?86-*-linux*)
+ VTV_SUPPORTED=yes
+ ;;
+ powerpc*-*-linux*)
+ ;;
+ sparc*-*-linux*)
+ ;;
+ arm*-*-linux*)
+ ;;
What about powerpc, sparc and arm? Why are they mentioned here if no
actual decision is made about support?
This is more a practical consideration: it's the middle of summer
break. Let's error on the side of caution for the moment, and get
this in causing minimal disruption on a convenient platform that I can
verify myself easily.
On a practical note, the libsanitizer acceptance
criteria was/is as above, seems sensible to do the same thing with
libvtv.
Once this is in trunk, let a million flowers bloom! There is no
reason specific platform/target maintainers can't enable it at their
leisure on a per-setup manner and when they can verify testresults
easily.
Agreed. The comments I had have been already addressed by Caroline,
AFAICT. Once she has that, the patch can go in.
Diego.
Re: [PATCH/Merge Request] Vtable Verification feature.
I have made all the requested changes. I am in the process of
bootstrapping and running the regressions one last time. Assuming the
bootstrapping regression tests pass, is this ok to commit?
-- Caroline
cmt...@google.com
On Tue, Aug 6, 2013 at 2:45 PM, Diego Novillo dnovi...@google.com wrote:
On Tue, Aug 6, 2013 at 2:39 PM, Benjamin De Kosnik b...@redhat.com wrote:
+# Filter out unsupported systems.
+case ${target} in
+ x86_64-*-linux* | i?86-*-linux*)
+ VTV_SUPPORTED=yes
+ ;;
+ powerpc*-*-linux*)
+ ;;
+ sparc*-*-linux*)
+ ;;
+ arm*-*-linux*)
+ ;;
What about powerpc, sparc and arm? Why are they mentioned here if no
actual decision is made about support?
This is more a practical consideration: it's the middle of summer
break. Let's error on the side of caution for the moment, and get
this in causing minimal disruption on a convenient platform that I can
verify myself easily.
On a practical note, the libsanitizer acceptance
criteria was/is as above, seems sensible to do the same thing with
libvtv.
Once this is in trunk, let a million flowers bloom! There is no
reason specific platform/target maintainers can't enable it at their
leisure on a per-setup manner and when they can verify testresults
easily.
Agreed. The comments I had have been already addressed by Caroline,
AFAICT. Once she has that, the patch can go in.
Diego.
Re: [PATCH/Merge Request] Vtable Verification feature.
On Tue, Aug 6, 2013 at 2:50 PM, Caroline Tice cmt...@google.com wrote: I have made all the requested changes. I am in the process of bootstrapping and running the regressions one last time. Assuming the bootstrapping regression tests pass, is this ok to commit? Sure. Diego.
Re: [PATCH/Merge Request] Vtable Verification feature.
This looks almost ready to commit. Some comments below: Once this is committed, you should write a blurb in GCC's home page describing the contribution. === --- libgcc/vtv_start_preinit.c (revision 0) +++ libgcc/vtv_start_preinit.c (revision 0) @@ -0,0 +1,68 @@ +/* Copyright (C) 2012, 2013 +Free Software Foundation, Inc. + +This file is part of GCC. + +GCC is free software; you can redistribute it and/or modify it under +the terms of the GNU General Public License as published by the Free +Software Foundation; either version 3, or (at your option) any later +version. + +GCC is distributed in the hope that it will be useful, but WITHOUT ANY +WARRANTY; without even the implied warranty of MERCHANTABILITY or +FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License +for more details. + +You should have received a copy of the GNU General Public License +along with GCC; see the file COPYING3. If not see +http://www.gnu.org/licenses/. */ Shouldn't this file have the GPL+exception license? + +/* This file is part of the vtable verification feature (for a + detailed description of the feature, see comments in + tree-vtable-verify.c). The vtable verification feature creates s/tree-vtable-verify.c/vtable-verify.c/ +GCC is distributed in the hope that it will be useful, but WITHOUT ANY +WARRANTY; without even the implied warranty of MERCHANTABILITY or +FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License +for more details. Ditto this one and all the other new files in libgcc/ + +You should have received a copy of the GNU General Public License +along with GCC; see the file COPYING3. If not see +http://www.gnu.org/licenses/. */ + +/* This file is part of the vtable verification feature (for a + detailed description of the feature, see comments in + tree-vtable-verify.c). The vtable verification feature creates s/tree-vtable-verify.c/vtable-verify.c/ +You should have received a copy of the GNU General Public License +along with GCC; see the file COPYING3. If not see +http://www.gnu.org/licenses/. */ + +/* This file is part of the vtable verification feature (for a + detailed description of the feature, see comments in + tree-vtable-verify.c). The vtable verification feature creates s/tree-vtable-verify.c/vtable-verify.c/ +You should have received a copy of the GNU General Public License +along with GCC; see the file COPYING3. If not see +http://www.gnu.org/licenses/. */ + +/* This file is part of the vtable verification feature (for a + detailed description of the feature, see comments in + tree-vtable-verify.c). The vtable verification feature creates s/tree-vtable-verify.c/vtable-verify.c/ + The vtable verification feature is controlled by the flag + '-fvtable-verify='. There are three flavors of this: + '-fvtable-verify=std', '-fvtable-verify=preinit', and + '-fvtable-verify=none'. If the option '-fvtable-verfy=preinit' is + used, then our constructor initialization function gets put into the + preinit array. This is necessary if there are data sets that need + to be built very early in execution. If the constructor + initialization function gets put into the preinit array, the we also + add calls to __VLTChangePermission at the beginning and end of the + function. The call at the beginning sets the permissions on the + data sets and vtable map variables to read/write, and the one at the + end makes them read-only. If the '-fvtable-verify=std' option is + used, the constructor initialization functions are executed at their + normal time, and the __VLTChangePermission calls are handled + differently (see the comments in libstdc++-v3/libsupc++/vtv_rts.cc). + The option '-fvtable-verify=none' turns off vtable verification. This part of the documentation, dealing with compiler flags and user-visible features should also be in doc/invoke.texi. +#include config.h +#include system.h +#include coretypes.h +#include tm.h +#include tree.h +#include tm_p.h +#include basic-block.h +#include output.h +#include tree-flow.h +#include tree-dump.h +#include tree-pass.h +#include timevar.h +#include cfgloop.h +#include flags.h +#include tree-inline.h +#include tree-scalar-evolution.h +#include diagnostic-core.h +#include gimple-pretty-print.h +#include toplev.h +#include langhooks.h +#include tree-ssa-propagate.h Have you made sure you actually need all these headers? There are some that look superfluous (like tree-ssa-propagate.h). + +#include vtable-verify.h + +unsigned num_vtable_map_nodes = 0; +bool any_verification_calls_generated = false; +int total_num_virtual_calls = 0; +int total_num_verified_vcalls = 0; I suppose all these are needed in the global namespace? I tested a couple and it seems to be case. If not, please make them static. Ideally, these would go in
Re: [PATCH/Merge Request] Vtable Verification feature.
On Mon, 2013-08-05 at 15:24 -0700, Diego Novillo wrote:
This looks almost ready to commit. Some comments below:
[...]
+/* Definition of this optimization pass. */
+
+struct gimple_opt_pass pass_vtable_verify =
+{
+ {
+ GIMPLE_PASS,
+ vtable-verify, /* name */
+ OPTGROUP_NONE,/* optinfo_flags */
+ gate_tree_vtable_verify, /* gate */
+ vtable_verify_main, /* execute */
+ NULL, /* sub */
+ NULL, /* next */
+ 0,/* static_pass_number */
+ TV_VTABLE_VERIFICATION, /* tv_id */
+ PROP_cfg | PROP_ssa, /* properties_required */
+ 0,/* properties_provided */
+ 0,/* properties_destroyed */
+ 0,/* todo_flags_start */
+ TODO_update_ssa /* todo_flags_finish */
+ }
+};
So, David M (CC'd) has just pulled the rug from under you a little
(Looks like I wasn't CCed, but I happened to see this given we were
chatting on IRC about it; have added myself to CC now)
bit. The pass manager now has a different API. The changes are not
too drastic, but you'll need to rework the registration of the pass.
(http://gcc.gnu.org/ml/gcc-patches/2013-08/msg00231.html).
I'm sorry for the inconvenience; a lot changed in trunk with these
changes.
A description of the API change can be seen in this mail:
http://gcc.gnu.org/ml/gcc-patches/2013-07/msg01259.html
and perhaps by reading:
https://github.com/davidmalcolm/gcc-refactoring-scripts/blob/master/test_refactor_passes.py
David wrote a refactoring tool to use, but this being a single pass it
should be easy to convert it manually. See the other passes, and I'm
sure David will be only too happy to help you if you run into trouble.
The script is refactor_passes.py within:
https://github.com/davidmalcolm/gcc-refactoring-scripts
I had a go at running the script on your branch; currently it fails
with:
File refactor_passes.py, line 372, in make_replacement2
assert extra != d[extra]
AssertionError
which is an overly terse way of reporting that a callback in an IPA pass
has the same identifier name as the name of the corresponding field,
which leads to the code breaking under the new API: specifically, I
believe the branch is missing the commit that's r201020 on trunk:
2013-07-18 David Malcolm dmalc...@redhat.com
* ipa-pure-const.c (generate_summary): Rename to...
(pure_const_generate_summary): ... this.
(see http://gcc.gnu.org/ml/gcc-patches/2013-07/msg00687.html for more
details on this one).
On running it on just vtable-verify.c thusly:
$ python refactor_passes.py ../src/gcc/vtable-verify.c
it generated the attached patch, which looks correct to me (I haven't
attempted to remerge the branches to see if it compiles, but it looks
like it should); I kept the generated ChangeLog in the patchfile.
You'll also need to change the extern decl in tree-pass.h:
extern struct gimple_opt_pass pass_vtable_verify;
to that of the factory function:
extern gimple_opt_pass *make_pass_vtable_verify (gcc::context *ctxt);
(the refactoring script will also do this, but currently in the branch
there are ~200 other passes that also get touched, so the specific
change would be lost in the noise).
Note that the vtable_verify pass appears to be single-instanced within
the pass tree. For future reference, if someone wants to run the script
on a *multi-instanced* pass, they'll need to supply a clone method (the
refactor script only adds them for those that it knows are
multi-instanced, using a hardcoded list). Writing a dummy clone method
is trivial, though.
Index: gcc/passes.def
===
--- gcc/passes.def (revision 201377)
+++ gcc/passes.def (working copy)
@@ -292,6 +292,7 @@ along with GCC; see the file COPYING3.
NEXT_PASS (pass_tm_memopt);
NEXT_PASS (pass_tm_edges);
POP_INSERT_PASSES ()
+ NEXT_PASS (pass_vtable_verify);
This will also need changes after David's changes.
If I reading this right, I don't think so - I think the above ought to
just work; r201505 updated the meaning of NEXT_PASS (actually for
pass-instances.def) so that it will use make_PASS_NAME i.e. this becomes
a call to make_pass_vtable_verify (wrapped in other stuff).
Hope this is helpful; sorry again for the inconvenience.
Dave
diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index f357e85..f1d8aed 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,14 @@
+2013-08-05 David Malcolm dmalc...@redhat.com
+
+ Patch autogenerated by refactor_passes.py from
+ https://github.com/davidmalcolm/gcc-refactoring-scripts
+ revision 03fe39476a4c4ea450b49e087cfa817b5f92021e
+
+ * vtable-verify.c (pass_vtable_verify):
Re: [PATCH/Merge Request] Vtable Verification feature.
On Thu, Aug 1, 2013 at 1:19 PM, Benjamin De Kosnik b...@redhat.com wrote: You'll need a GWP to do the merge (Maybe Diego, Jason, Richard Henderson?) and then add yourself to MAINTAINERS for libvtv. Eh, no. We need a GWP to approve the final patch, but Caroline can and should do the merge herself. The easiest approach is to generate a patch on an svn checkout of trunk. Caroline and I discussed this earlier today. It goes something like this (on a git client): $ git checkout -b vtv vtv $ git merge origin/trunk ... fix merge conflicts ... $ git commit $ git diff trunk patch ... Get rid of all the ChangeLog.vtv and configure diffs ... ... Use git log to find the svn revision REV for the local trunk git tree ... $ git co svn://gcc.gnu.org/gcc/trunk@REV $ patch -p1 patch $ svn add ...new directories and files... At the end of this you'll have a checkout of trunk with the vtv changes in. You can now test it and send the final patch to the list for a GWP to review. Diego.
Re: [PATCH/Merge Request] Vtable Verification feature.
Nice to see! I have created (with some help) a git branch on gcc.gnu.org to contain the vtable verification feature work. This work is now well integrated with GCC trunk, and the sources are in a good state for future work. I believe all previous review comments have been addressed. The runtime bits are OK to me. Thanks for all your work on this. The feature is disabled by default, and disabled on non-linux OSes. Regression testing on linux is showing great results (no regressions). I can confirm these results on x86_64/linux. There is a wiki page at http://gcc.gnu.org/wiki/vtv that contains information about how to use the feature, as well as links to the Feature Proposal, User's Guide, and the presentation on this feature at last year's GNU Tools Cauldron. This work has been ongoing since GCC 4.7 and is now ready to merge into trunk for the GCC 4.9.0 release. I would like permission to merge this in (and or information on the best way to proceed from here). Thanks. You'll need a GWP to do the merge (Maybe Diego, Jason, Richard Henderson?) and then add yourself to MAINTAINERS for libvtv. -benjamin You'll need a GWP to do the merge, and then add yourself to MAINTAINERS for libvtv.
