diff mbox

[5/7] arm64: keep .altinstructions and .altinstr_replacement

Message ID 20171129234442.655-6-samitolvanen@google.com (mailing list archive)
State New, archived
Headers show

Commit Message

Sami Tolvanen Nov. 29, 2017, 11:44 p.m. UTC
Make sure the linker doesn't remove .altinstructions or
.altinstr_replacement when CONFIG_LD_DEAD_CODE_DATA_ELIMINATION is
enabled.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 arch/arm64/kernel/vmlinux.lds.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Nick Desaulniers Nov. 29, 2017, 11:57 p.m. UTC | #1
On Wed, Nov 29, 2017 at 3:44 PM, Sami Tolvanen <samitolvanen@google.com> wrote:
> Make sure the linker doesn't remove .altinstructions or
> .altinstr_replacement when CONFIG_LD_DEAD_CODE_DATA_ELIMINATION is
> enabled.

This sounds like a bug in the original implementation of
CONFIG_LD_DEAD_CODE_DATA_ELIMINATION? If so, this can likely get
merged regardless of the rest of the patchset, ie. gold support.
Nicholas Piggin Nov. 30, 2017, 1:58 a.m. UTC | #2
On Wed, 29 Nov 2017 15:57:53 -0800
Nick Desaulniers <ndesaulniers@google.com> wrote:

> On Wed, Nov 29, 2017 at 3:44 PM, Sami Tolvanen <samitolvanen@google.com> wrote:
> > Make sure the linker doesn't remove .altinstructions or
> > .altinstr_replacement when CONFIG_LD_DEAD_CODE_DATA_ELIMINATION is
> > enabled.  
> 
> This sounds like a bug in the original implementation of
> CONFIG_LD_DEAD_CODE_DATA_ELIMINATION? If so, this can likely get
> merged regardless of the rest of the patchset, ie. gold support.

Yes, also patch 3/7 probably (thanks for doing these, btw). The
LD_DCDE option is actually been broken upstream thanks to lack
of time (which is a known issue, this is why no arch enables it).

Basically it just needs a bit more effort to go through and
ensure nothing is discarded incorrectly, and all new sections
are placed into output sections, before each arch is enabled.

(Comparing `readelf -S` before/after is a way to spot bugs.)

So yes please if it's not too much trouble, could you remove
the "gold" name from the generic patch and put it at the front
of the series with this arm64 patch. Possibly then you could
also do a 3rd patch to allow arm64 to select it if it's working
with gcc?

Thanks,
Nick
Nick Desaulniers Nov. 30, 2017, 5 p.m. UTC | #3
On Wed, Nov 29, 2017 at 5:58 PM, Nicholas Piggin <npiggin@gmail.com> wrote:
> (Comparing `readelf -S` before/after is a way to spot bugs.)

Do you have a script that can be used to diff 2 `readelf -S`, look for
missing references, and report an error? Might be easier to rerun that
every so often and see what's missing.
Sami Tolvanen Nov. 30, 2017, 5:48 p.m. UTC | #4
On Thu, Nov 30, 2017 at 11:58:27AM +1000, Nicholas Piggin wrote:
> So yes please if it's not too much trouble, could you remove
> the "gold" name from the generic patch and put it at the front
> of the series with this arm64 patch.

Sure, I'll do this in v2.

> Possibly then you could also do a 3rd patch to allow arm64 to
> select it if it's working with gcc?

These patches only fix the issues I ran into with clang and gold
when testing on a single device.  I feel like more testing would
be needed before enabling this by default for arm64.

Sami
Nicholas Piggin Dec. 1, 2017, 12:36 a.m. UTC | #5
On Thu, 30 Nov 2017 09:00:35 -0800
Nick Desaulniers <ndesaulniers@google.com> wrote:

> On Wed, Nov 29, 2017 at 5:58 PM, Nicholas Piggin <npiggin@gmail.com> wrote:
> > (Comparing `readelf -S` before/after is a way to spot bugs.)  
> 
> Do you have a script that can be used to diff 2 `readelf -S`, look for
> missing references, and report an error? Might be easier to rerun that
> every so often and see what's missing.

I don't, I was just doing it by hand.

Thanks,
Nick
diff mbox

Patch

diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
index 7da3e5c366a0..15479995869c 100644
--- a/arch/arm64/kernel/vmlinux.lds.S
+++ b/arch/arm64/kernel/vmlinux.lds.S
@@ -138,11 +138,11 @@  SECTIONS
 	. = ALIGN(4);
 	.altinstructions : {
 		__alt_instructions = .;
-		*(.altinstructions)
+		KEEP(*(.altinstructions))
 		__alt_instructions_end = .;
 	}
 	.altinstr_replacement : {
-		*(.altinstr_replacement)
+		KEEP(*(.altinstr_replacement))
 	}
 
 	. = ALIGN(PAGE_SIZE);