* [PATCH][GOLD] Fix segment alignment
@ 2010-11-18 21:53 Doug Kwan (關振德)
2010-11-18 22:53 ` Ian Lance Taylor
0 siblings, 1 reply; 4+ messages in thread
From: Doug Kwan (關振德) @ 2010-11-18 21:53 UTC (permalink / raw)
To: Ian Lance Taylor, binutils; +Cc: Markus Trippelsdorf
[-- Attachment #1: Type: text/plain, Size: 884 bytes --]
Hi Ian,
This patch fixes the alignment problem reported by Markus. I
looked at declarations of Expression::eval_with_dot() and
Expression::eval_maybe_dot() in script.h. According to comments in
there, the location *RESULT_ALIGNMENT is written only if the existing
value there is smaller than the alignment of the expression. So I
think it is incorrect that Binary_<operator>::value() sets
*EEI->RESULT_ALIGNMENT_POINTER unconditionally. Furthermore, the old
code looks a little suspicious to me, why do we only use
right_alignment? Should the KEEP_LEFT case use left_alignment instead?
This is tested on x86_64-unknown-linux-gnu.
-Doug
2010-11-18 Doug Kwan <dougkwan@google.com>
* expression.cc (BINARY_EXPRESSION): Initalize left_alignment
and right_alignment to be zero. Store result alignment only if it is
greater than existing alignment.
[-- Attachment #2: patch-align.txt --]
[-- Type: text/plain, Size: 1806 bytes --]
Index: gold/expression.cc
===================================================================
RCS file: /cvs/src/src/gold/expression.cc,v
retrieving revision 1.17
diff -u -u -p -r1.17 expression.cc
--- gold/expression.cc 1 Oct 2010 15:02:33 -0000 1.17
+++ gold/expression.cc 18 Nov 2010 21:39:33 -0000
@@ -399,18 +399,19 @@ class Binary_expression : public Express
value(const Expression_eval_info* eei) \
{ \
Output_section* left_section; \
- uint64_t left_alignment; \
+ uint64_t left_alignment = 0; \
uint64_t left = this->left_value(eei, &left_section, \
&left_alignment); \
Output_section* right_section; \
- uint64_t right_alignment; \
+ uint64_t right_alignment = 0; \
uint64_t right = this->right_value(eei, &right_section, \
&right_alignment); \
if (KEEP_RIGHT && left_section == NULL && right_section != NULL) \
{ \
if (eei->result_section_pointer != NULL) \
*eei->result_section_pointer = right_section; \
- if (eei->result_alignment_pointer != NULL) \
+ if (eei->result_alignment_pointer != NULL \
+ && right_alignment > *eei->result_alignment_pointer) \
*eei->result_alignment_pointer = right_alignment; \
} \
else if (KEEP_LEFT \
@@ -419,7 +420,8 @@ class Binary_expression : public Express
{ \
if (eei->result_section_pointer != NULL) \
*eei->result_section_pointer = left_section; \
- if (eei->result_alignment_pointer != NULL) \
+ if (eei->result_alignment_pointer != NULL \
+ && right_alignment > *eei->result_alignment_pointer) \
*eei->result_alignment_pointer = right_alignment; \
} \
else if ((WARN || left_section != right_section) \
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH][GOLD] Fix segment alignment
2010-11-18 21:53 [PATCH][GOLD] Fix segment alignment Doug Kwan (關振德)
@ 2010-11-18 22:53 ` Ian Lance Taylor
2010-11-18 23:56 ` Fwd: " Doug Kwan (關振德)
0 siblings, 1 reply; 4+ messages in thread
From: Ian Lance Taylor @ 2010-11-18 22:53 UTC (permalink / raw)
To: Doug Kwan (關振德); +Cc: binutils, Markus Trippelsdorf
"Doug Kwan (關振德)" <dougkwan@google.com> writes:
> 2010-11-18 Doug Kwan <dougkwan@google.com>
>
> * expression.cc (BINARY_EXPRESSION): Initalize left_alignment
> and right_alignment to be zero. Store result alignment only if it is
> greater than existing alignment.
> @@ -419,7 +420,8 @@ class Binary_expression : public Express
> { \
> if (eei->result_section_pointer != NULL) \
> *eei->result_section_pointer = left_section; \
> - if (eei->result_alignment_pointer != NULL) \
> + if (eei->result_alignment_pointer != NULL \
> + && right_alignment > *eei->result_alignment_pointer) \
> *eei->result_alignment_pointer = right_alignment; \
> } \
Hmmm, this is clearly a bug in my code. This should be left_alignment
rather than right_alignment.
This is OK with that change.
Thanks for tracking this down.
Ian
^ permalink raw reply [flat|nested] 4+ messages in thread
* Fwd: [PATCH][GOLD] Fix segment alignment
2010-11-18 22:53 ` Ian Lance Taylor
@ 2010-11-18 23:56 ` Doug Kwan (關振德)
2010-11-19 11:40 ` Tristan Gingold
0 siblings, 1 reply; 4+ messages in thread
From: Doug Kwan (關振德) @ 2010-11-18 23:56 UTC (permalink / raw)
To: Tristan Gingold; +Cc: binutils, Ian Lance Taylor, Markus Trippelsdorf
Hi Tristan,
Could you please merge this commit?
http://www.cygwin.com/ml/binutils-cvs/2010-11/msg00122.html
I verified that binutils-2.20.90 has the same problem and it can be
fixed by this patch.
Thanks
-Doug
---------- Forwarded message ----------
From: Ian Lance Taylor <iant@google.com>
Date: Thu, Nov 18, 2010 at 2:53 PM
Subject: Re: [PATCH][GOLD] Fix segment alignment
To: "Doug Kwan (關振德)" <dougkwan@google.com>
Cc: binutils <binutils@sourceware.org>, Markus Trippelsdorf
<markus@trippelsdorf.de>
"Doug Kwan (關振德)" <dougkwan@google.com> writes:
> 2010-11-18 Doug Kwan <dougkwan@google.com>
>
> * expression.cc (BINARY_EXPRESSION): Initalize left_alignment
> and right_alignment to be zero. Store result alignment only if it is
> greater than existing alignment.
> @@ -419,7 +420,8 @@ class Binary_expression : public Express
> { \
> if (eei->result_section_pointer != NULL) \
> *eei->result_section_pointer = left_section; \
> - if (eei->result_alignment_pointer != NULL) \
> + if (eei->result_alignment_pointer != NULL \
> + && right_alignment > *eei->result_alignment_pointer) \
> *eei->result_alignment_pointer = right_alignment; \
> } \
Hmmm, this is clearly a bug in my code. This should be left_alignment
rather than right_alignment.
This is OK with that change.
Thanks for tracking this down.
Ian
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH][GOLD] Fix segment alignment
2010-11-18 23:56 ` Fwd: " Doug Kwan (關振德)
@ 2010-11-19 11:40 ` Tristan Gingold
0 siblings, 0 replies; 4+ messages in thread
From: Tristan Gingold @ 2010-11-19 11:40 UTC (permalink / raw)
To: Doug Kwan (關振德)
Cc: binutils, Ian Lance Taylor, Markus Trippelsdorf
On Nov 19, 2010, at 12:56 AM, Doug Kwan (關振德) wrote:
> Hi Tristan,
>
> Could you please merge this commit?
Done.
>
> http://www.cygwin.com/ml/binutils-cvs/2010-11/msg00122.html
>
> I verified that binutils-2.20.90 has the same problem and it can be
> fixed by this patch.
>
> Thanks
>
> -Doug
>
>
> ---------- Forwarded message ----------
> From: Ian Lance Taylor <iant@google.com>
> Date: Thu, Nov 18, 2010 at 2:53 PM
> Subject: Re: [PATCH][GOLD] Fix segment alignment
> To: "Doug Kwan (關振德)" <dougkwan@google.com>
> Cc: binutils <binutils@sourceware.org>, Markus Trippelsdorf
> <markus@trippelsdorf.de>
>
>
> "Doug Kwan (關振德)" <dougkwan@google.com> writes:
>
>> 2010-11-18 Doug Kwan <dougkwan@google.com>
>>
>> * expression.cc (BINARY_EXPRESSION): Initalize left_alignment
>> and right_alignment to be zero. Store result alignment only if it is
>> greater than existing alignment.
>
>> @@ -419,7 +420,8 @@ class Binary_expression : public Express
>> { \
>> if (eei->result_section_pointer != NULL) \
>> *eei->result_section_pointer = left_section; \
>> - if (eei->result_alignment_pointer != NULL) \
>> + if (eei->result_alignment_pointer != NULL \
>> + && right_alignment > *eei->result_alignment_pointer) \
>> *eei->result_alignment_pointer = right_alignment; \
>> } \
>
> Hmmm, this is clearly a bug in my code. This should be left_alignment
> rather than right_alignment.
>
> This is OK with that change.
>
> Thanks for tracking this down.
>
> Ian
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-11-19 11:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-11-18 21:53 [PATCH][GOLD] Fix segment alignment Doug Kwan (關振德)
2010-11-18 22:53 ` Ian Lance Taylor
2010-11-18 23:56 ` Fwd: " Doug Kwan (關振德)
2010-11-19 11:40 ` Tristan Gingold
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).