| Message ID | 561CFEB4.1000708@foss.arm.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Oct 13, 2015 at 1:53 PM, Ramana Radhakrishnan <ramana.radhakrishnan@foss.arm.com> wrote: > > > > On 12/10/15 21:44, Jeff Law wrote: >> On 10/09/2015 03:17 AM, Ramana Radhakrishnan wrote: >>> This started as a Friday afternoon project ... >>> >>> It turned out enabling VTV for AArch64 and ARM was a matter of fixing >>> PR67868 which essentially comes from building libvtv with section >>> anchors turned on. The problem was that the flow of control from >>> output_object_block through to switch_section did not have the same >>> special casing for the vtable section that exists in >>> assemble_variable. >> That's some ugly code. You might consider factoring that code into a function and just calling it from both places. Your version doesn't seem to handle PECOFF, so I'd probably refactor from assemble_variable. >> > > I was a bit lazy as I couldn't immediately think of a target that would want PECOFF, section anchors and VTV. That combination seems to be quite rare, anyway point taken on the refactor. > > Ok if no regressions ? Ping. Ramana > >>> >>> However both these failures also occur on x86_64 - so I'm content to >>> declare victory on AArch64 as far as basic enablement goes. >> Cool. >> >>> >>> 1. Are the generic changes to varasm.c ok ? 2. Can we take the >>> AArch64 support in now, given this amount of testing ? Marcus / >>> Caroline ? 3. Any suggestions / helpful debug hints for VTV debugging >>> (other than turning VTV_DEBUG on and inspecting trace) ? >> I think that with refactoring they'd be good to go. No opinions on the AArch64 specific question -- call for the AArch64 maintainers. >> >> Good to see someone hacking on vtv. It's in my queue to look at as well. > > Yeah figuring out more about vtv is also in my background queue. > > regards > Ramana > > PR other/67868 > > * varasm.c (assemble_variable): Move special vtv handling to.. > (handle_vtv_comdat_sections): .. here. New function. > (output_object_block): Handle vtv sections. > > libvtv/Changelog > > * configure.tgt: Support aarch64 and arm.
Sending this again, as the mailer daemon rejected it last time.... It looks good to me, but I can only approve the files that go into libvtv. In (belated) response to your earlier question about debugging vtv problems, there's a fair amount of useful info for debugging in the User's Guide, off the wiki page (https://gcc.gnu.org/wiki/vtv). If you're already read that and have further questions, let me know... -- Caroline cmtice@google.com On Mon, Oct 19, 2015 at 4:39 PM, Caroline Tice <cmtice@google.com> wrote: > It looks good to me, but I can only approve the files that go into libvtv. > > In (belated) response to your earlier question about debugging vtv problems, > there's a fair amount of useful info for debugging in the User's Guide, off > the wiki page (https://gcc.gnu.org/wiki/vtv). If you're already read that > and have further questions, let me know... > > > -- Caroline > cmtice@google.com > > On Mon, Oct 19, 2015 at 1:54 AM, Ramana Radhakrishnan > <ramana.gcc@googlemail.com> wrote: >> >> On Tue, Oct 13, 2015 at 1:53 PM, Ramana Radhakrishnan >> <ramana.radhakrishnan@foss.arm.com> wrote: >> > >> > >> > >> > On 12/10/15 21:44, Jeff Law wrote: >> >> On 10/09/2015 03:17 AM, Ramana Radhakrishnan wrote: >> >>> This started as a Friday afternoon project ... >> >>> >> >>> It turned out enabling VTV for AArch64 and ARM was a matter of fixing >> >>> PR67868 which essentially comes from building libvtv with section >> >>> anchors turned on. The problem was that the flow of control from >> >>> output_object_block through to switch_section did not have the same >> >>> special casing for the vtable section that exists in >> >>> assemble_variable. >> >> That's some ugly code. You might consider factoring that code into a >> >> function and just calling it from both places. Your version doesn't seem to >> >> handle PECOFF, so I'd probably refactor from assemble_variable. >> >> >> > >> > I was a bit lazy as I couldn't immediately think of a target that would >> > want PECOFF, section anchors and VTV. That combination seems to be quite >> > rare, anyway point taken on the refactor. >> > >> > Ok if no regressions ? >> >> Ping. >> >> Ramana >> >> > >> >>> >> >>> However both these failures also occur on x86_64 - so I'm content to >> >>> declare victory on AArch64 as far as basic enablement goes. >> >> Cool. >> >> >> >>> >> >>> 1. Are the generic changes to varasm.c ok ? 2. Can we take the >> >>> AArch64 support in now, given this amount of testing ? Marcus / >> >>> Caroline ? 3. Any suggestions / helpful debug hints for VTV debugging >> >>> (other than turning VTV_DEBUG on and inspecting trace) ? >> >> I think that with refactoring they'd be good to go. No opinions on the >> >> AArch64 specific question -- call for the AArch64 maintainers. >> >> >> >> Good to see someone hacking on vtv. It's in my queue to look at as >> >> well. >> > >> > Yeah figuring out more about vtv is also in my background queue. >> > >> > regards >> > Ramana >> > >> > PR other/67868 >> > >> > * varasm.c (assemble_variable): Move special vtv handling to.. >> > (handle_vtv_comdat_sections): .. here. New function. >> > (output_object_block): Handle vtv sections. >> > >> > libvtv/Changelog >> > >> > * configure.tgt: Support aarch64 and arm. > >
On 10/13/2015 06:53 AM, Ramana Radhakrishnan wrote: > > > > On 12/10/15 21:44, Jeff Law wrote: >> On 10/09/2015 03:17 AM, Ramana Radhakrishnan wrote: >>> This started as a Friday afternoon project ... >>> >>> It turned out enabling VTV for AArch64 and ARM was a matter of fixing >>> PR67868 which essentially comes from building libvtv with section >>> anchors turned on. The problem was that the flow of control from >>> output_object_block through to switch_section did not have the same >>> special casing for the vtable section that exists in >>> assemble_variable. >> That's some ugly code. You might consider factoring that code into a function and just calling it from both places. Your version doesn't seem to handle PECOFF, so I'd probably refactor from assemble_variable. >> > > I was a bit lazy as I couldn't immediately think of a target that would want PECOFF, section anchors and VTV. That combination seems to be quite rare, anyway point taken on the refactor. > > Ok if no regressions ? > >>> >>> However both these failures also occur on x86_64 - so I'm content to >>> declare victory on AArch64 as far as basic enablement goes. >> Cool. >> >>> >>> 1. Are the generic changes to varasm.c ok ? 2. Can we take the >>> AArch64 support in now, given this amount of testing ? Marcus / >>> Caroline ? 3. Any suggestions / helpful debug hints for VTV debugging >>> (other than turning VTV_DEBUG on and inspecting trace) ? >> I think that with refactoring they'd be good to go. No opinions on the AArch64 specific question -- call for the AArch64 maintainers. >> >> Good to see someone hacking on vtv. It's in my queue to look at as well. > > Yeah figuring out more about vtv is also in my background queue. > > regards > Ramana > > PR other/67868 > > * varasm.c (assemble_variable): Move special vtv handling to.. > (handle_vtv_comdat_sections): .. here. New function. > (output_object_block): Handle vtv sections. > > libvtv/Changelog > > * configure.tgt: Support aarch64 and arm. OK for the trunk. Sorry for the delay, I was out on the PTO the latter half of last week. jeff
diff --git a/gcc/varasm.c b/gcc/varasm.c index f1564bc..62ad863 100644 --- a/gcc/varasm.c +++ b/gcc/varasm.c @@ -127,6 +127,7 @@ static void asm_output_aligned_bss (FILE *, tree, const char *, #endif /* BSS_SECTION_ASM_OP */ static void mark_weak (tree); static void output_constant_pool (const char *, tree); +static void handle_vtv_comdat_section (section *, const_tree); /* Well-known sections, each one associated with some sort of *_ASM_OP. */ section *text_section; @@ -2230,56 +2231,10 @@ assemble_variable (tree decl, int top_level ATTRIBUTE_UNUSED, assemble_noswitch_variable (decl, name, sect, align); else { - /* The following bit of code ensures that vtable_map - variables are not only in the comdat section, but that - each variable has its own unique comdat name. If this - code is removed, the variables end up in the same section - with a single comdat name. - - FIXME: resolve_unique_section needs to deal better with - decls with both DECL_SECTION_NAME and DECL_ONE_ONLY. Once - that is fixed, this if-else statement can be replaced with - a single call to "switch_to_section (sect)". */ + /* Special-case handling of vtv comdat sections. */ if (sect->named.name && (strcmp (sect->named.name, ".vtable_map_vars") == 0)) - { -#if defined (OBJECT_FORMAT_ELF) - targetm.asm_out.named_section (sect->named.name, - sect->named.common.flags - | SECTION_LINKONCE, - DECL_NAME (decl)); - in_section = sect; -#elif defined (TARGET_PECOFF) - /* Neither OBJECT_FORMAT_PE, nor OBJECT_FORMAT_COFF is set here. - Therefore the following check is used. - In case a the target is PE or COFF a comdat group section - is created, e.g. .vtable_map_vars$foo. The linker places - everything in .vtable_map_vars at the end. - - A fix could be made in - gcc/config/i386/winnt.c: i386_pe_unique_section. */ - if (TARGET_PECOFF) - { - char *name; - - if (TREE_CODE (DECL_NAME (decl)) == IDENTIFIER_NODE) - name = ACONCAT ((sect->named.name, "$", - IDENTIFIER_POINTER (DECL_NAME (decl)), NULL)); - else - name = ACONCAT ((sect->named.name, "$", - IDENTIFIER_POINTER (DECL_COMDAT_GROUP (DECL_NAME (decl))), - NULL)); - - targetm.asm_out.named_section (name, - sect->named.common.flags - | SECTION_LINKONCE, - DECL_NAME (decl)); - in_section = sect; - } -#else - switch_to_section (sect); -#endif - } + handle_vtv_comdat_section (sect, decl); else switch_to_section (sect); if (align > BITS_PER_UNIT) @@ -7329,7 +7284,14 @@ output_object_block (struct object_block *block) /* Switch to the section and make sure that the first byte is suitably aligned. */ - switch_to_section (block->sect); + /* Special case VTV comdat sections similar to assemble_variable. */ + if (SECTION_STYLE (block->sect) == SECTION_NAMED + && block->sect->named.name + && (strcmp (block->sect->named.name, ".vtable_map_vars") == 0)) + handle_vtv_comdat_section (block->sect, block->sect->named.decl); + else + switch_to_section (block->sect); + assemble_align (block->alignment); /* Define the values of all anchors relative to the current section @@ -7772,4 +7734,56 @@ default_asm_output_ident_directive (const char *ident_str) fprintf (asm_out_file, "%s\"%s\"\n", ident_asm_op, ident_str); } + +/* This function ensures that vtable_map variables are not only + in the comdat section, but that each variable has its own unique + comdat name. Without this the variables end up in the same section + with a single comdat name. + + FIXME: resolve_unique_section needs to deal better with + decls with both DECL_SECTION_NAME and DECL_ONE_ONLY. Once + that is fixed, this if-else statement can be replaced with + a single call to "switch_to_section (sect)". */ + +static void +handle_vtv_comdat_section (section *sect, const_tree decl) +{ +#if defined (OBJECT_FORMAT_ELF) + targetm.asm_out.named_section (sect->named.name, + sect->named.common.flags + | SECTION_LINKONCE, + DECL_NAME (decl)); + in_section = sect; +#elif defined (TARGET_PECOFF) + /* Neither OBJECT_FORMAT_PE, nor OBJECT_FORMAT_COFF is set here. + Therefore the following check is used. + In case a the target is PE or COFF a comdat group section + is created, e.g. .vtable_map_vars$foo. The linker places + everything in .vtable_map_vars at the end. + + A fix could be made in + gcc/config/i386/winnt.c: i386_pe_unique_section. */ + if (TARGET_PECOFF) + { + char *name; + + if (TREE_CODE (DECL_NAME (decl)) == IDENTIFIER_NODE) + name = ACONCAT ((sect->named.name, "$", + IDENTIFIER_POINTER (DECL_NAME (decl)), NULL)); + else + name = ACONCAT ((sect->named.name, "$", + IDENTIFIER_POINTER (DECL_COMDAT_GROUP (DECL_NAME (decl))), + NULL)); + + targetm.asm_out.named_section (name, + sect->named.common.flags + | SECTION_LINKONCE, + DECL_NAME (decl)); + in_section = sect; + } +#else + switch_to_section (sect); +#endif +} + #include "gt-varasm.h" diff --git a/libvtv/configure.tgt b/libvtv/configure.tgt index e543371..adcff5c 100644 --- a/libvtv/configure.tgt +++ b/libvtv/configure.tgt @@ -37,6 +37,10 @@ case "${target}" in sparc*-*-linux*) ;; arm*-*-linux*) + VTV_SUPPORTED=yes + ;; + aarch64*-*-linux*) + VTV_SUPPORTED=yes ;; x86_64-*-darwin[1]* | i?86-*-darwin[1]*) ;;