Page MenuHomePhabricator

ObjectFileELF: Add support for arbitrarily named code sections

Authored by kbaladurin on Mar 28 2018, 1:24 PM.



ObjectFileELF assumes that code section has ".text" name. There is an exception for kalimba toolchain that can use arbitrary names, but other toolchains also could use arbitrary names for code sections. For example, corert uses separate section for compiled managed code. As lldb doesn't recognize such section it leads to problem with breakpoints on arm, because debugger cannot determine instruction set (arm/thumb) and uses incorrect breakpoint opcode that breaks program execution.

This change allows debugger to correctly handle such code sections. We assume that section is a code section if it has SHF_EXECINSTR flag set and has SHT_PROGBITS type.

Diff Detail


Event Timeline

kbaladurin created this revision.Mar 28 2018, 1:24 PM
davide requested changes to this revision.Mar 28 2018, 1:31 PM
davide added a subscriber: davide.

This needs a testcase.

This revision now requires changes to proceed.Mar 28 2018, 1:31 PM

Quick test case where we have an ELF file with a .text section and with another executable section with something in it that we can set breakpoints in. Easiest to make an ELF file that fits the bill, and then run obj2yaml on it. You check that into a test case directory and have the test case run yaml2obj on it and run the test.

labath added a subscriber: labath.Mar 29 2018, 3:42 AM

We already have section dumping code in the lldb-test utility. Extending the output to dump the detected section type seems like a good (and simple) thing to do.

Thank you! What is more preferable add python or lit test?

lldb-test for this purpose will be great. there should be examples in lit/.

kbaladurin updated this revision to Diff 140609.Apr 2 2018, 1:56 AM

Thank you! I've extended lldb-test utility to dump section type:

Showing 6 sections
  Index: 0
  Type: regular
  VM size: 0
  File size: 0
  Index: 1
  Type: code
  Name: .text
  VM size: 8
  File size: 8
  Index: 2
  Type: code
  Name: __codesection
  VM size: 8
  File size: 8
  Index: 3
  Type: elf-symbol-table
  Name: .symtab
  VM size: 0
  File size: 16

and added lit test to check section type and arm test in python test suite to check breakpoints in arbitrary named thumb code sections.

davide added a comment.Apr 2 2018, 7:58 AM

I think this is almost ready to go in modulo minors. I'll let also @labath comment on it. Thanks for your contribution!
Do you need somebody to commit this on your behalf?

2 ↗(On Diff #140609)

nit: arbitrary.

1950–1954 ↗(On Diff #140609)

can you add a comment explaining why this is needed?

kbaladurin updated this revision to Diff 140628.EditedApr 2 2018, 8:24 AM

@davide thank you! I've updated diff. Yes, I need somebody to commit this, as I haven't commit access.

kbaladurin marked 2 inline comments as done.Apr 2 2018, 8:28 AM
clayborg requested changes to this revision.Apr 2 2018, 8:32 AM
clayborg added inline comments.
1 ↗(On Diff #140628)

Will this work with all compilers we currently run the test suite with? I would assume with will work with GCC and Clang at least. IF not, we might need to make a lldbtest.h file that any test case can use and use a macro here?

30 ↗(On Diff #140628)

Why did you take static off of this function? Please remove this change, or change this function to get the section type from the section itself and not require the argument.

This revision now requires changes to proceed.Apr 2 2018, 8:32 AM
kbaladurin added inline comments.Apr 2 2018, 9:03 AM
1 ↗(On Diff #140628)

What compilers do we use with test suite? This construction works fine with gcc and clang. Is it enough for us?

30 ↗(On Diff #140628)

I change it to static method to use it in lldb-test. There is similar static methods in Value and Scalar classes: Value::GetValueTypeAsCString and Scalar::GetValueTypeAsCString. Is non static method more preferable for us in this case?

davide accepted this revision.Apr 2 2018, 9:07 AM

LGTM. I'll commit for you once Greg reviews it again.

1 ↗(On Diff #140628)

I think this is fine.
BTW, if we really want to abstract this, the place is not lldb but llvm.
We already have an header for the purpose.


But again, I don't think this is needed.

30 ↗(On Diff #140628)

I think what you did was correct. Greg?

The patch looks fine to me. I think I'd make the arm test (I guess that is interesting because of the bit 0 twiddling ?) a non-execution test, but this is fine as well.

29 ↗(On Diff #140628)

Do you think there's any added value in making sure that the breakpoint is *hit* (instead of just making sure that it's resolved to the right file address). If it's the latter we could make this a non-execution test and have it run everywhere (e.g., via lldb-test breakpoint) instead of just on arm hardware.

clayborg accepted this revision.Apr 2 2018, 1:21 PM
clayborg added inline comments.
30 ↗(On Diff #140628)

Woops, I missed the extra "Section::" that was added. You indeed did do this right... I thought in my mind that the "Section::" was there prior to this change and we were making the function a member function. My bad.

This revision is now accepted and ready to land.Apr 2 2018, 1:21 PM
kbaladurin added inline comments.Apr 3 2018, 1:28 AM
29 ↗(On Diff #140628)

I think we need to make sure that breakpoint is *hit* and program can successfully continue execution after it, because it is resolved to right address in both cases:

lldb without this patch:

$ llvm-arm/bin/lldb thumb-bp 
(lldb) target create "thumb-bp"
Current executable set to 'thumb-bp' (arm).
(lldb) b bp.c:3
Breakpoint 1: where = thumb-bp`f + 6 at bp.c:3, address = 0x000103f6
(lldb) r
Process 22192 launched: '/home/kbaladurin/thumb-bp' (arm)
Process 22192 stopped
* thread #1: tid = 22192, 0x000103d0 thumb-bp`__libc_csu_init + 32, name = 'thumb-bp', stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
    frame #0: 0x000103d0 thumb-bp`__libc_csu_init + 32
->  0x103d0 <+32>: ldr    r3, [r5], #4
    0x103d4 <+36>: mov    r2, r9
    0x103d6 <+38>: mov    r1, r8
    0x103d8 <+40>: mov    r0, r7
(lldb) disas -n f
    0x103f0 <+0>:  strmi  r11, [r1], -r2, lsl #1
    0x103f4 <+4>:  stmdals r1, {r0, r12, pc}
    0x103f8 <+8>:  .long  0x91003001                ; unknown opcode
    0x103fc <+12>: ldrbmi r11, [r0, -r2]!

lldb with this patch:

$ llvm-arm-patch/bin/lldb thumb-bp 
(lldb) target create "thumb-bp"
Current executable set to 'thumb-bp' (arm).
(lldb) b bp.c:3
Breakpoint 1: where = thumb-bp`f + 6 at bp.c:3, address = 0x000103f6
(lldb) r
Process 22282 launched: '/home/kbaladurin/thumb-bp' (arm)
Process 22282 stopped
* thread #1: tid = 22282, 0x000103f6 thumb-bp`f(a=10) + 6 at bp.c:3, name = 'thumb-bp', stop reason = breakpoint 1.1
    frame #0: 0x000103f6 thumb-bp`f(a=10) + 6 at bp.c:3
   1   	__attribute__((section("__codesection")))
   2   	int f(int a) {
-> 3   	  return a + 1; // Set break point at this line.
   4   	}
   6   	int main() {
   7   	  return f(10);
(lldb) disas
    0x103f0 <+0>:  sub    sp, #0x8
    0x103f2 <+2>:  mov    r1, r0
    0x103f4 <+4>:  str    r0, [sp, #0x4]
->  0x103f6 <+6>:  ldr    r0, [sp, #0x4]
    0x103f8 <+8>:  adds   r0, #0x1
    0x103fa <+10>: str    r1, [sp]
    0x103fc <+12>: add    sp, #0x8
    0x103fe <+14>: bx     lr

In both cases resolved address is 0x103f6 that is right according to the debug information:

Line Number Statements:
 [0x00000025]  Extended opcode 2: set Address to 0x103f0
 [0x0000002c]  Special opcode 6: advance Address by 0 to 0x103f0 and Line by 1 to 2
 [0x0000002d]  Set column to 10
 [0x0000002f]  Set prologue_end to true
 [0x00000030]  Special opcode 90: advance Address by 6 to 0x103f6 and Line by 1 to 3

But lldb uses arm breakpoint in the first case and thumb one in the second.

labath accepted this revision.Apr 3 2018, 2:19 AM

I see, thanks for explaining that.

Is it ready to commit?

Yes, I take it you need someone to check it in?

Yes, as I haven't commit access. Thank you!

I've committed this as r331173. I've had to revert the lldb-test changes, as something similar has been implemented already, and I've merged your lldb-test test with an existing section types test.

This revision was automatically updated to reflect the committed changes.

Thank you very much!