On 9/14/18 12:39 PM, Bernd Edlinger wrote:
> Hi,
>
> this is an upate of the string-merge section, it is based on the V2-STRING_CST
> semantic patch series, which was finally installed yesterday.
> It merges single-byte string constants with or without terminating NUL.
> The patch has the same Ada and C test cases that were already in the V1 patch.
>
> Thus there are no pre-requisite patches necessary at this time.
>
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?
>
>
> Thanks
> Bernd.
>
>
> patch-merge-strings-v2.diff
>
> gcc:
> 2018-08-27 Bernd Edlinger <bernd.edlin...@hotmail.de>
>
> * varasm.c (output_constant): Add new parameter merge_strings.
> Make strings properly zero terminated in merge string sections.
> (mergeable_string_section): Don't fail if the last char is non-zero.
> (assemble_variable_contents): Handle merge string sections.
> (assemble_variable): Likewise.
> (assemble_constant_contents): Likewise.
> (output_constant_def_contents): Likewise.
>
> testsuite:
> 2018-08-27 Bernd Edlinger <bernd.edlin...@hotmail.de>
>
> * gnat.dg/string_merge1.adb: New test.
> * gnat.dg/string_merge2.adb: New test.
> * gcc.dg/merge-all-constants-1.c: Adjust test.
> * gcc.dg/merge-all-constants-2.c: New test.
Thanks for you patience.
>
> diff -Npur gcc/varasm.c gcc/varasm.c
> --- gcc/varasm.c 2018-08-26 15:02:43.157905415 +0200
> +++ gcc/varasm.c 2018-08-26 17:57:26.488494866 +0200
> @@ -111,7 +111,8 @@ static int compare_constant (const tree,
> static void output_constant_def_contents (rtx);
> static void output_addressed_constants (tree);
> static unsigned HOST_WIDE_INT output_constant (tree, unsigned HOST_WIDE_INT,
> - unsigned int, bool);
> + unsigned int, bool,
> + bool = false);
Given there's less than 10 call sites in total, avoid defaulting the
argument and just pass it in unconditionally. Similarly for
assemble_constant_contents which has just a couple calls.
> static void globalize_decl (tree);
> static bool decl_readonly_section_1 (enum section_category);
> #ifdef BSS_SECTION_ASM_OP
> @@ -804,8 +805,8 @@ mergeable_string_section (tree decl ATTR
> && TREE_CODE (decl) == STRING_CST
> && TREE_CODE (TREE_TYPE (decl)) == ARRAY_TYPE
> && align <= 256
> - && (len = int_size_in_bytes (TREE_TYPE (decl))) > 0
> - && TREE_STRING_LENGTH (decl) >= len)
> + && (len = int_size_in_bytes (TREE_TYPE (decl))) >= 0
> + && TREE_STRING_LENGTH (decl) == len)
Not sure why you want to test for >= 0 here. > 0 seems sufficient,
though I guess there's no harm in the = 0 case.
> @@ -835,7 +836,7 @@ mergeable_string_section (tree decl ATTR
> if (j == unit)
> break;
> }
> - if (i == len - unit)
> + if (i == len - unit || (unit == 1 && i == len))
> {
> sprintf (name, "%s.str%d.%d", prefix,
> modesize / 8, (int) (align / 8));
> @@ -2316,7 +2317,9 @@ assemble_variable (tree decl, int top_le
> switch_to_section (sect);
> if (align > BITS_PER_UNIT)
> ASM_OUTPUT_ALIGN (asm_out_file, floor_log2 (align / BITS_PER_UNIT));
> - assemble_variable_contents (decl, name, dont_output_data);
> + assemble_variable_contents (decl, name, dont_output_data,
> + sect->common.flags & SECTION_MERGE
> + && sect->common.flags & SECTION_STRINGS);
I believe our style guidelines would call for parenthesis around the
multi-line expression (where you check SECTION_MERGE and SECTION_STRINGS).
> @@ -3522,10 +3526,13 @@ output_constant_def_contents (rtx symbol
> || (VAR_P (decl) && DECL_IN_CONSTANT_POOL (decl))
> ? DECL_ALIGN (decl)
> : symtab_node::get (decl)->definition_alignment ());
> - switch_to_section (get_constant_section (exp, align));
> + section *sect = get_constant_section (exp, align);
> + switch_to_section (sect);
> if (align > BITS_PER_UNIT)
> ASM_OUTPUT_ALIGN (asm_out_file, floor_log2 (align / BITS_PER_UNIT));
> - assemble_constant_contents (exp, XSTR (symbol, 0), align);
> + assemble_constant_contents (exp, XSTR (symbol, 0), align,
> + sect->common.flags & SECTION_MERGE
> + && sect->common.flags & SECTION_STRINGS);
Similarly, you need some parens here.
I think with the changing of the default parm to a normal parm and the
style changes this is OK for the trunk. Given it's independent of
everything else going on, go ahead and commit it when you've fixed up
those minor issues.
jeff
