* [PATCH] PR gold/20462: Fix bogus layout on ARM with linker script using PHDRS clause
@ 2016-08-12 0:14 Roland McGrath
2016-08-12 16:54 ` Cary Coutant
0 siblings, 1 reply; 3+ messages in thread
From: Roland McGrath @ 2016-08-12 0:14 UTC (permalink / raw)
To: binutils
See https://sourceware.org/bugzilla/show_bug.cgi?id=20462 for the test
case.
I'm not at all sure this fix is correct. It fixes both the minimal
test case I put in the bug, and my real case (which uses the same
linker script). It causes no regressions in 'make check-gold' in
a trunk build with --target=arm-eabi.
Here's what I figured out:
* On the first pass, the layout comes out just fine.
* ARM always tries relaxation (unless -r).
* In Target_arm::do_relax it calls Target_arm::fix_exidx_coverage and
decides that constituted some relaxation happening, so a second pass
always happens when there are any .ARM.exidx sections.
* In the second pass, Script_sections::expected_segment_count sees
this->segments_created_ and short-circuits, so it returns 0 instead
of this->phdrs_elements_->size().
* Ergo, SIZEOF_HEADERS omits the phdrs in the second pass and
addresses go backwards, making everything unhappy.
Script_sections::expected_segment_count is the only thing that tests
this->segments_created_. It's not really clear to me if this ought
to be part of the state that Script_sections::release_segments (sole
caller Layout::clean_up_after_relaxation) resets, as my change here
does. An alternate fix would be to reverse the order of the first two
checks in Script_sections::expected_segment_count so that it always
returns this->phdrs_elements_->size() when this->saw_phdrs_clause().
(I haven't tried that on my test cases.)
If this is indeed a good fix, may I commit it to trunk and 2.27?
If not, is my other suggestion right or is there a different fix?
Thanks,
Roland
gold/
2016-08-11 Roland McGrath <roland@hack.frob.com>
PR gold/20462
* script-sections.cc (Script_sections::release_segments):
Reset this->segments_created_.
diff --git a/gold/script-sections.cc b/gold/script-sections.cc
index 96c68de..bf25391 100644
--- a/gold/script-sections.cc
+++ b/gold/script-sections.cc
@@ -4459,6 +4459,7 @@ Script_sections::release_segments()
++p)
(*p)->release_segment();
}
+ this->segments_created_ = false;
}
// Print the SECTIONS clause to F for debugging.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] PR gold/20462: Fix bogus layout on ARM with linker script using PHDRS clause
2016-08-12 0:14 [PATCH] PR gold/20462: Fix bogus layout on ARM with linker script using PHDRS clause Roland McGrath
@ 2016-08-12 16:54 ` Cary Coutant
2016-08-12 16:57 ` Roland McGrath
0 siblings, 1 reply; 3+ messages in thread
From: Cary Coutant @ 2016-08-12 16:54 UTC (permalink / raw)
To: Roland McGrath; +Cc: binutils
> I'm not at all sure this fix is correct. It fixes both the minimal
> test case I put in the bug, and my real case (which uses the same
> linker script). It causes no regressions in 'make check-gold' in
> a trunk build with --target=arm-eabi.
>
> Here's what I figured out:
> * On the first pass, the layout comes out just fine.
> * ARM always tries relaxation (unless -r).
> * In Target_arm::do_relax it calls Target_arm::fix_exidx_coverage and
> decides that constituted some relaxation happening, so a second pass
> always happens when there are any .ARM.exidx sections.
> * In the second pass, Script_sections::expected_segment_count sees
> this->segments_created_ and short-circuits, so it returns 0 instead
> of this->phdrs_elements_->size().
> * Ergo, SIZEOF_HEADERS omits the phdrs in the second pass and
> addresses go backwards, making everything unhappy.
>
> Script_sections::expected_segment_count is the only thing that tests
> this->segments_created_. It's not really clear to me if this ought
> to be part of the state that Script_sections::release_segments (sole
> caller Layout::clean_up_after_relaxation) resets, as my change here
> does. An alternate fix would be to reverse the order of the first two
> checks in Script_sections::expected_segment_count so that it always
> returns this->phdrs_elements_->size() when this->saw_phdrs_clause().
> (I haven't tried that on my test cases.)
>
> If this is indeed a good fix, may I commit it to trunk and 2.27?
> If not, is my other suggestion right or is there a different fix?
Yes, I think this is the right fix. OK for trunk and 2.27 branch.
Thanks!
-cary
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-08-12 16:57 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-12 0:14 [PATCH] PR gold/20462: Fix bogus layout on ARM with linker script using PHDRS clause Roland McGrath
2016-08-12 16:54 ` Cary Coutant
2016-08-12 16:57 ` Roland McGrath
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).