Re: [Gold] Question about the incorrect variable alignment if the alignment is greater than the page size

[Alexander Ivchenko <aivchenk at gmail dot com>]

2013/6/5 Cary Coutant <ccoutant@google.com>:
>>> In the test, I recommend that rather than using
>>> large_symbol_alignment.sh you just write your main function to verify
>>> that the symbols are correctly aligned.  And put a space between the
>>> right parenthesis and "int".
>>
>> Done. I had to make artificial volatile variables because gcc always tried to
>> delete the true "if" branch as a dead code (in O0 as well), eventhough
>> I check in gdb
>> that the condition is true:
>>
>> 40|   if ((reinterpret_cast<uintptr_t>(&aligned_16k_var) & 16383) != 0
>> 41|       || (reinterpret_cast<uintptr_t>(&aligned_8k_var) & 8191) != 0
>> 42|       || (reinterpret_cast<uintptr_t>(&aligned_4k_var) & 4095) != 0)
>> 43|       return 1;
>> 44+>  return 0;
>>
>> (gdb) p (reinterpret_cast<uintptr_t>(&aligned_16k_var) & 16383) != 0
>> $2 = true
>>
>> Gcc probably assumes that if the variable been declared with attribute
>> aligned then it will
>> certanly have that alignment...
>
> I'd suggest using a noinline function instead, like this:
>
> bool
> is_aligned(const int& var, int align) __attribute__((noinline));
>
> bool
> is_aligned(const int& var, int align)
> {
>   return (reinterpret_cast<uintptr_t>(&var) & (align - 1)) == 0;
> }
>
> ...
>   if (!is_aligned(aligned_16k_var, 16384)
>       || !is_aligned(aligned_8k_var, 8192)
>       || !is_aligned(aligned_4k_var, 4096))
>       return 1;
> ...
> With that, you don't need to force -O0 in the Makefile (and you didn't
> have to force -g anyway), so you can simplify Makefile.am:
>
> check_PROGRAMS += large_symbol_alignment
> large_symbol_alignment_SOURCES = large_symbol_alignment.cc
> large_symbol_alignment_DEPENDENCIES = gcctestdir/ld
> large_symbol_alignment_LDFLAGS = -Bgcctestdir/
>
> Also, I suggest reordering the variable declarations like this:
>
> __attribute__ ((aligned(8192)))  int aligned_8k_var;
> __attribute__ ((aligned(4096)))  int aligned_4k_var;
> __attribute__ ((aligned(16384))) int aligned_16k_var;
>
> so that there will be less chance of an accidental alignment that
> makes the test pass even when the linker is broken.
>
> +int
> +main (int argc __attribute__ ((unused)),
> +      char** argv __attribute__ ((unused)))
>
> Either "main()" or "main(int, char**)" (no space before the paren) is
> all you need here.


Yep, that is much better, thanks, I changed it.

> For future reference, when sending a patch, you shouldn't include the
> diffs for ChangeLog or for generated files like Makefile.in. Just put
> your ChangeLog entry at the top of the patch.

The patch is attached (Makefile.in is not included in the diff).
As before make check-gold passes on x86_64-unknown-linux-gnu.

thanks,
Alexander

> -cary

Attachment: large_alignment_fix_03.patch
Description: Binary data