Re: [gold] Merging string literals with bigger alignment

[Cary Coutant <ccoutant@google.com>]

>> The view you're looking at will already be aligned correctly, so I
>> don't think this is necessary (i.e., init_align_modulo will always be
>> 0).
>
> No, that could not be true, because, as far as I understand, pdata
> here points at our internal storage that
> does not have to comply with the alignment requirements. So all we can
> do here is to assume that its alignment
> is correct and then check the alignment of each string within this
> section relatively to that initial modulo.
> (I encounter that when pdata for .rodata.str1.16 section from libc.a
> had 8 alignment, but, of course, that section
> itself in libc.a was correctly 16-aligned)

Sorry, you're right. File_read::find_or_make_view will align the file
data if requested (by copying into a temporary buffer if necessary),
but only to the target's natural alignment (which seems suspicious --
Ian, shouldn't it align to the host's natural alignment?).

>> I think you'll also need to change
>> Stringpool_template<Stringpool_char>::Stringpool_eq::operator() to
>> check the alignment that you've stored in the Hashkey. Imagine one
>> section that adds a string with alignment 1, and a subsequent section
>> that adds the same string with alignment 4 -- when adding the string
>> the second time, you'll match the first, and keep the 1-byte
>> alignment. Ideally, you'd just update the alignment so there's only
>> one copy of the string at the larger alignment, but that will only
>> work if you're optimizing the string tables (-O2).
>
> I know that now two equal strings with different alignments will not
> be merged (BFD also does not merge them together).
> But to improve that we also need to change
> Merge_section_properties::eq() so mergable string sections with
> different alignments
> would fall into the same Output_merge_string_data (we also need to
> accordingly update some asserts, hash function for
> Merge_section_properties etc.).
> Since BFD doesn't do it, I want to make it as a separate subsequent
> patch, if you don't mind of course..

I understand that add_merge_input_section will group merge sections by
their alignment, so that 1-byte aligned strings and 4-byte aligned
strings will go into different merge sections. I'm OK with that, and
am not asking you to change that.

My point is that you've altered the Stringpool API to present an
interface that allows strings with different alignment requirements to
be entered into the same pool, but that it will ignore the larger
alignment if the pool already contains the same string with a smaller
alignment. I'd suggest: (a) make it do the right thing by entering the
string again with the larger alignment, or (b) change the API to
disallow this possibility. If you do (a), you could also have it
coalesce the identical strings into the one with the larger alignment
when optimizing the string table. If you do (b), you could either (b1)
have it assert that the alignment is the same for every string added
to the pool, or (b2) move the alignment from add_with_length to the
Stringpool constructor.

Since you know that add_merge_input_section will ensure that all
strings in one pool will have the same alignment, I'd suggest (b2).
The alignment is already passed to the Output_merge_string
constructor, and can easily be passed to the initializer for
stringpool_.

-cary