| Message ID | 20171116065829.30889-1-nathan@nathanrossi.com |
|---|---|
| State | New |
| Headers | show |
| Series | MicroBlaze use default ident output generation | expand |
On 11/15/2017 11:58 PM, Nathan Rossi wrote: > Remove the MicroBlaze specific TARGET_ASM_OUTPUT_IDENT definition, and > use the default. > > This resolves issues associated with the use of the .sdata2 operation in > cases where emitted assembly after the ident output is incorrectly in > the .sdata2 section instead of .text or any other expected section. > Which results in assembly failures including operations with symbols > across different segments. > > gcc/ChangeLog > > 2017-11-16 Nathan Rossi <nathan@nathanrossi.com> > > PR target/83013 > * config/microblaze/microblaze-protos.h > (microblaze_asm_output_ident): Delete > * config/microblaze/microblaze.c (microblaze_asm_output_ident): Delete > * config/microblaze/microblaze.h (TARGET_ASM_OUTPUT_IDENT): Default But isn't the purpose of the override to force certain small-enough objects into the .sdata2 section and by removing the override aren't you losing that capability? It does seem like the override is broken in that it changes the current section behind the back of the generic code. Wouldn't a better fix be to ensure that the override arranges to switch back to whatever the current section is? Perhaps using .pushsection and .popsection would help here? jeff
On 11/15/2017 10:58 PM, Nathan Rossi wrote: > Remove the MicroBlaze specific TARGET_ASM_OUTPUT_IDENT definition, and > use the default. > > This resolves issues associated with the use of the .sdata2 operation in > cases where emitted assembly after the ident output is incorrectly in > the .sdata2 section instead of .text or any other expected section. > Which results in assembly failures including operations with symbols > across different segments. Can you give me an example where this causes a problem?
On 18 November 2017 at 04:25, Jeff Law <law@redhat.com> wrote: > On 11/15/2017 11:58 PM, Nathan Rossi wrote: >> Remove the MicroBlaze specific TARGET_ASM_OUTPUT_IDENT definition, and >> use the default. >> >> This resolves issues associated with the use of the .sdata2 operation in >> cases where emitted assembly after the ident output is incorrectly in >> the .sdata2 section instead of .text or any other expected section. >> Which results in assembly failures including operations with symbols >> across different segments. >> >> gcc/ChangeLog >> >> 2017-11-16 Nathan Rossi <nathan@nathanrossi.com> >> >> PR target/83013 >> * config/microblaze/microblaze-protos.h >> (microblaze_asm_output_ident): Delete >> * config/microblaze/microblaze.c (microblaze_asm_output_ident): Delete >> * config/microblaze/microblaze.h (TARGET_ASM_OUTPUT_IDENT): Default > But isn't the purpose of the override to force certain small-enough > objects into the .sdata2 section and by removing the override aren't you > losing that capability? > > It does seem like the override is broken in that it changes the current > section behind the back of the generic code. > > Wouldn't a better fix be to ensure that the override arranges to switch > back to whatever the current section is? Perhaps using .pushsection and > .popsection would help here? > That would be a better fix, however I sent this change first as it seemed it might be preferred to remove the target specific behavior instead of fixing it. Since it is the only target that actually uses the TARGET_ASM_OUTPUT_IDENT to change the output asm content (others either define the default or have a function that calls the default). But I can sort out a patch that fixes the behavior instead if that is preferred? Regards, Nathan
On 18 November 2017 at 04:30, Michael Eager <eager@eagerm.com> wrote: > On 11/15/2017 10:58 PM, Nathan Rossi wrote: >> >> Remove the MicroBlaze specific TARGET_ASM_OUTPUT_IDENT definition, and >> use the default. >> >> This resolves issues associated with the use of the .sdata2 operation in >> cases where emitted assembly after the ident output is incorrectly in >> the .sdata2 section instead of .text or any other expected section. >> Which results in assembly failures including operations with symbols >> across different segments. > > > Can you give me an example where this causes a problem? Sure, I filed a GCC bugzilla with a sample code file that exhibits the cross-segment symbol issue. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83013 There was also an older bug I saw before filing the above which might be the same issue though I have not verified that. Just looking at the .s files from the linked binutils issue appears to show the same issue however with .rodata instead of .sdata2. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63261 Regards, Nathan
On 18 November 2017 at 22:13, Nathan Rossi <nathan@nathanrossi.com> wrote: > On 18 November 2017 at 04:25, Jeff Law <law@redhat.com> wrote: >> On 11/15/2017 11:58 PM, Nathan Rossi wrote: >>> Remove the MicroBlaze specific TARGET_ASM_OUTPUT_IDENT definition, and >>> use the default. >>> >>> This resolves issues associated with the use of the .sdata2 operation in >>> cases where emitted assembly after the ident output is incorrectly in >>> the .sdata2 section instead of .text or any other expected section. >>> Which results in assembly failures including operations with symbols >>> across different segments. >>> >>> gcc/ChangeLog >>> >>> 2017-11-16 Nathan Rossi <nathan@nathanrossi.com> >>> >>> PR target/83013 >>> * config/microblaze/microblaze-protos.h >>> (microblaze_asm_output_ident): Delete >>> * config/microblaze/microblaze.c (microblaze_asm_output_ident): Delete >>> * config/microblaze/microblaze.h (TARGET_ASM_OUTPUT_IDENT): Default >> But isn't the purpose of the override to force certain small-enough >> objects into the .sdata2 section and by removing the override aren't you >> losing that capability? >> >> It does seem like the override is broken in that it changes the current >> section behind the back of the generic code. >> >> Wouldn't a better fix be to ensure that the override arranges to switch >> back to whatever the current section is? Perhaps using .pushsection and >> .popsection would help here? >> > > That would be a better fix, however I sent this change first as it > seemed it might be preferred to remove the target specific behavior > instead of fixing it. Since it is the only target that actually uses > the TARGET_ASM_OUTPUT_IDENT to change the output asm content (others > either define the default or have a function that calls the default). > > But I can sort out a patch that fixes the behavior instead if that is preferred? Ping. Should I sort out a patch which uses the push/pop of the section or is this patch preferred? Regards, Nathan
On 01/09/2018 01:26 AM, Nathan Rossi wrote: > On 18 November 2017 at 22:13, Nathan Rossi <nathan@nathanrossi.com> wrote: >> On 18 November 2017 at 04:25, Jeff Law <law@redhat.com> wrote: >>> On 11/15/2017 11:58 PM, Nathan Rossi wrote: >>>> Remove the MicroBlaze specific TARGET_ASM_OUTPUT_IDENT definition, and >>>> use the default. >>>> >>>> This resolves issues associated with the use of the .sdata2 operation in >>>> cases where emitted assembly after the ident output is incorrectly in >>>> the .sdata2 section instead of .text or any other expected section. >>>> Which results in assembly failures including operations with symbols >>>> across different segments. >>>> >>>> gcc/ChangeLog >>>> >>>> 2017-11-16 Nathan Rossi <nathan@nathanrossi.com> >>>> >>>> PR target/83013 >>>> * config/microblaze/microblaze-protos.h >>>> (microblaze_asm_output_ident): Delete >>>> * config/microblaze/microblaze.c (microblaze_asm_output_ident): Delete >>>> * config/microblaze/microblaze.h (TARGET_ASM_OUTPUT_IDENT): Default >>> But isn't the purpose of the override to force certain small-enough >>> objects into the .sdata2 section and by removing the override aren't you >>> losing that capability? >>> >>> It does seem like the override is broken in that it changes the current >>> section behind the back of the generic code. >>> >>> Wouldn't a better fix be to ensure that the override arranges to switch >>> back to whatever the current section is? Perhaps using .pushsection and >>> .popsection would help here? >>> >> >> That would be a better fix, however I sent this change first as it >> seemed it might be preferred to remove the target specific behavior >> instead of fixing it. Since it is the only target that actually uses >> the TARGET_ASM_OUTPUT_IDENT to change the output asm content (others >> either define the default or have a function that calls the default). >> >> But I can sort out a patch that fixes the behavior instead if that is preferred? > > Ping. Should I sort out a patch which uses the push/pop of the section > or is this patch preferred? I'd approve a push/pop. THe current patch as-is seems broken to me. jeff
diff --git a/gcc/config/microblaze/microblaze-protos.h b/gcc/config/microblaze/microblaze-protos.h index 747ef35971..3d3e8c5a64 100644 --- a/gcc/config/microblaze/microblaze-protos.h +++ b/gcc/config/microblaze/microblaze-protos.h @@ -51,7 +51,6 @@ extern int microblaze_regno_ok_for_base_p (int, int); extern HOST_WIDE_INT microblaze_initial_elimination_offset (int, int); extern void microblaze_declare_object (FILE *, const char *, const char *, const char *, int); -extern void microblaze_asm_output_ident (const char *); extern int microblaze_legitimate_pic_operand (rtx); extern bool microblaze_tls_referenced_p (rtx); extern int symbol_mentioned_p (rtx); diff --git a/gcc/config/microblaze/microblaze.c b/gcc/config/microblaze/microblaze.c index 7487523877..379f4c4d7f 100644 --- a/gcc/config/microblaze/microblaze.c +++ b/gcc/config/microblaze/microblaze.c @@ -3377,30 +3377,6 @@ microblaze_eh_return (rtx op0) emit_insn (gen_movsi (gen_rtx_MEM (Pmode, stack_pointer_rtx), op0)); } -/* Queue an .ident string in the queue of top-level asm statements. - If the string size is below the threshold, put it into .sdata2. - If the front-end is done, we must be being called from toplev.c. - In that case, do nothing. */ -void -microblaze_asm_output_ident (const char *string) -{ - const char *section_asm_op; - int size; - char *buf; - - if (symtab->state != PARSING) - return; - - size = strlen (string) + 1; - if (size <= microblaze_section_threshold) - section_asm_op = SDATA2_SECTION_ASM_OP; - else - section_asm_op = READONLY_DATA_SECTION_ASM_OP; - - buf = ACONCAT ((section_asm_op, "\n\t.ascii \"", string, "\\0\"\n", NULL)); - symtab->finalize_toplevel_asm (build_string (strlen (buf), buf)); -} - static void microblaze_elf_asm_init_sections (void) { diff --git a/gcc/config/microblaze/microblaze.h b/gcc/config/microblaze/microblaze.h index 59cc1cc2e3..06155d3163 100644 --- a/gcc/config/microblaze/microblaze.h +++ b/gcc/config/microblaze/microblaze.h @@ -705,7 +705,7 @@ do { \ #define STRING_ASM_OP "\t.asciz\t" #undef TARGET_ASM_OUTPUT_IDENT -#define TARGET_ASM_OUTPUT_IDENT microblaze_asm_output_ident +#define TARGET_ASM_OUTPUT_IDENT default_asm_output_ident_directive /* Default to -G 8 */ #ifndef MICROBLAZE_DEFAULT_GVALUE