Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#5139 closed feature request (fixed)

Output ELF .size directives

Reported by: tibbe Owned by: tibbe
Priority: normal Milestone: 7.2.1
Component: Compiler Version: 7.0.3
Keywords: Cc: johan.tibell@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

This lets tools like Linux's perf events find out symbols and display them in generated reports.

Attachments (4)

0001-Output-ELF-.size-directives-for-functions.patch (3.4 KB) - added by tibbe 9 years ago.
dynHelloWorld.s.noelf (4.2 KB) - added by tibbe 9 years ago.
Assembler of dynHelloWorld.hs without my patch
dynHelloWorld.s.elf (4.5 KB) - added by tibbe 9 years ago.
Assembler of dynHelloWorld.hs with my patch
0001-Output-ELF-.size-directives-for-functions.2.patch (1.2 KB) - added by tibbe 9 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 9 years ago by tibbe

Cc: johan.tibell@… added
Status: newpatch

Attached a patch against HEAD that implements this.

comment:2 Changed 9 years ago by tibbe

Before patch:

$ perf report
   75.28%            bench  ./bench                    [.] 0x000000002b612b
    7.11%            bench  /lib/libc-2.11.1.so        [.] memcpy
    5.47%            bench  ./bench                    [.] 0x0000000000438f
    3.47%            bench  ./bench                    [.] allocate
    1.86%            bench  ./bench                    [.] s1Ol_info
    1.65%            bench  ./bench                    [.] s1Ok_info
    0.62%            bench  ./bench                    [.] memcpy@plt
    0.48%            bench  ./bench                    [.] clearNurseries
    0.33%            bench  ./bench                    [.] evacuate1
    0.24%            bench  ./bench                    [.] GarbageCollect

After patch:

$ perf report
    61.25%            bench  ./bench               [.] stg_newArrayzh
    13.25%            bench  ./bench               [.] stg_copyArrayzh
     7.62%            bench  /lib/libc-2.11.1.so   [.] memcpy
     4.31%            bench  ./bench               [.] s23r_info
     3.36%            bench  ./bench               [.] allocate
     3.31%            bench  ./bench               [.] s23q_info
     0.89%            bench  ./bench               [.] r20J_info
     0.79%            bench  ./bench               [.] memcpy@plt
     0.74%            bench  ./bench               [.] clearNurseries
     0.32%            bench  ./bench               [.] scavenge_mutable_list

N.B. Long term we need to make sure that CLabels get the right CLabelType. Currently labels that point at code often have the DataLabel type. This is probably due to TABLES_NEXT_TO_CODE.

comment:3 Changed 9 years ago by simonmar

Milestone: 7.2.1
Owner: set to igloo
Priority: normalhigh

Looks good. Ian, could you push please?

comment:4 Changed 9 years ago by igloo

Resolution: fixed
Status: patchclosed

Applied, thanks

comment:5 Changed 9 years ago by igloo

Owner: igloo deleted
Priority: highnormal
Resolution: fixed
Status: closednew

Sorry, a workflow failure meant I hadn't actually validated this, and it turns out that it breaks dynHelloWorld(dyn):

dynHelloWorld: internal error: evacuate(static): strange closure type -385875968
    (GHC version 7.1.20110422 for x86_64_unknown_linux)
    Please report this as a GHC bug:  http://www.haskell.org/ghc/reportabug

I've reverted it (changeset:f1f3c4f50650110ad0f700d6566a44c515b0548f).

(gdb) p /x -385875968
$1 = 0xe9000000

comment:6 Changed 9 years ago by tibbe

Reproducing this wasn't as straightforward as I though i.e. make TEST=dynHelloWorld didn't work. Do I need to rebuild the stage 2 compiler in some particular way to be able to run the test?

Changed 9 years ago by tibbe

Attachment: dynHelloWorld.s.noelf added

Assembler of dynHelloWorld.hs without my patch

Changed 9 years ago by tibbe

Attachment: dynHelloWorld.s.elf added

Assembler of dynHelloWorld.hs with my patch

comment:7 Changed 9 years ago by tibbe

Owner: set to tibbe

I've attached the assembler output for dynHelloWorld.hs with and without my patch. Nothing sticks out. A bunch of labels have changed type from @object to @function, which is intended. I will have to continue digging. If someone who's more familiar with how dynamic compilation works would like to take a look just run:

$ diff -u dynHelloWorld.s.noelf dynHelloWorld.s.elf

comment:8 Changed 9 years ago by tibbe

I've triaged the issue a bit further. Adding the .size directive is enough to make dynHelloWorld fail:

diff --git a/compiler/nativeGen/X86/Ppr.hs b/compiler/nativeGen/X86/Ppr.hs
index 5fe78e1..f8f4cc1 100644
--- a/compiler/nativeGen/X86/Ppr.hs
+++ b/compiler/nativeGen/X86/Ppr.hs
@@ -87,7 +87,17 @@ pprNatCmmTop (CmmProc info lbl (ListGraph blocks)) =
                       <+> pprCLabel_asm (mkDeadStripPreventer $ entryLblToInfoLbl lbl)
                     else empty
 #endif
+   $$ pprSizeDecl (if null info then lbl else entryLblToInfoLbl lbl)
 
+-- | Output the ELF .size directive.
+pprSizeDecl :: CLabel -> Doc
+#if elf_OBJ_FORMAT
+pprSizeDecl lbl =
+    ptext (sLit "\t.size") <+> pprCLabel_asm lbl
+    <> ptext (sLit ", .-") <> pprCLabel_asm lbl
+#else
+pprSizeDecl _ = empty
+#endif
 
 pprBasicBlock :: NatBasicBlock Instr -> Doc
 pprBasicBlock (BasicBlock blockid instrs) =

comment:9 in reply to:  8 Changed 9 years ago by tibbe

Replying to tibbe:

I've triaged the issue a bit further. Adding the .size directive is enough to make dynHelloWorld fail:

diff --git a/compiler/nativeGen/X86/Ppr.hs b/compiler/nativeGen/X86/Ppr.hs
index 5fe78e1..f8f4cc1 100644
--- a/compiler/nativeGen/X86/Ppr.hs
+++ b/compiler/nativeGen/X86/Ppr.hs
@@ -87,7 +87,17 @@ pprNatCmmTop (CmmProc info lbl (ListGraph blocks)) =
                       <+> pprCLabel_asm (mkDeadStripPreventer $ entryLblToInfoLbl lbl)
                     else empty
 #endif
+   $$ pprSizeDecl (if null info then lbl else entryLblToInfoLbl lbl)
 
+-- | Output the ELF .size directive.
+pprSizeDecl :: CLabel -> Doc
+#if elf_OBJ_FORMAT
+pprSizeDecl lbl =
+    ptext (sLit "\t.size") <+> pprCLabel_asm lbl
+    <> ptext (sLit ", .-") <> pprCLabel_asm lbl
+#else
+pprSizeDecl _ = empty
+#endif
 
 pprBasicBlock :: NatBasicBlock Instr -> Doc
 pprBasicBlock (BasicBlock blockid instrs) =

I take that back, I forgot to rebuild the stage 1 compiler before rebuilding the RTS.

comment:10 Changed 9 years ago by tibbe

Status: newpatch

I was wrong, .size is not to blame, .type is. I've created a new patch with only the .size changes. It validates. The patch is a subset of the previous patch so it shouldn't require additional review, but feel free to review anyway.

I will create a separate ticket for the .type changes.

comment:11 Changed 9 years ago by igloo

Resolution: fixed
Status: patchclosed

Applied, thanks!

comment:12 Changed 9 years ago by simonmar

For future reference: the problem is almost certainly that the addition of .type and/or .size in some combination has triggered the generation of R_COPY relocations, which are bad news for the tables-next-to-code layout that GHC uses. More details in Commentary/PositionIndependentCode.

Note: See TracTickets for help on using tickets.