Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1645923 > unrolled thread

Re: [PATCH 7/7] DWARF: add the config option

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 7/7] DWARF: add the config option Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-19 23:00 +0200
    Re: [PATCH 7/7] DWARF: add the config option "H. Peter Anvin" <hpa@zytor.com> - 2017-05-19 23:10 +0200
      Re: [PATCH 7/7] DWARF: add the config option Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-19 23:30 +0200
        Re: [PATCH 7/7] DWARF: add the config option Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-19 23:40 +0200
          Re: [PATCH 7/7] DWARF: add the config option Andy Lutomirski <luto@kernel.org> - 2017-05-20 07:30 +0200
            Re: [PATCH 7/7] DWARF: add the config option Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-20 18:30 +0200
              Re: [PATCH 7/7] DWARF: add the config option Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-20 19:20 +0200
              Re: [PATCH 7/7] DWARF: add the config option "H.J. Lu" <hjl.tools@gmail.com> - 2017-05-20 22:10 +0200
                Re: [PATCH 7/7] DWARF: add the config option Andy Lutomirski <luto@kernel.org> - 2017-05-21 00:00 +0200
                  Re: [PATCH 7/7] DWARF: add the config option "H.J. Lu" <hjl.tools@gmail.com> - 2017-05-21 00:30 +0200
                    Re: [PATCH 7/7] DWARF: add the config option Jiri Kosina <jikos@kernel.org> - 2017-05-22 13:40 +0200
                      Re: [PATCH 7/7] DWARF: add the config option "H.J. Lu" <hjl.tools@gmail.com> - 2017-05-22 16:50 +0200
                Re: [PATCH 7/7] DWARF: add the config option "H. Peter Anvin" <hpa@zytor.com> - 2017-05-22 23:20 +0200
                  Re: [PATCH 7/7] DWARF: add the config option "H. Peter Anvin" <hpa@zytor.com> - 2017-05-22 23:50 +0200
                    Re: [PATCH 7/7] DWARF: add the config option Josh Poimboeuf <jpoimboe@redhat.com> - 2017-05-23 00:20 +0200
            Re: [PATCH 7/7] DWARF: add the config option Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-20 22:20 +0200
              Re: [PATCH 7/7] DWARF: add the config option Andy Lutomirski <luto@kernel.org> - 2017-05-21 00:00 +0200
                Re: [PATCH 7/7] DWARF: add the config option Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-21 01:10 +0200
                  Re: [PATCH 7/7] DWARF: add the config option Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-21 01:30 +0200
        Re: [PATCH 7/7] DWARF: add the config option Ingo Molnar <mingo@kernel.org> - 2017-05-22 13:20 +0200
          Re: [PATCH 7/7] DWARF: add the config option "H. Peter Anvin" <hpa@zytor.com> - 2017-05-22 23:30 +0200
            Re: [PATCH 7/7] DWARF: add the config option Jiri Kosina <jikos@kernel.org> - 2017-05-23 01:30 +0200
            Re: [PATCH 7/7] DWARF: add the config option Ingo Molnar <mingo@kernel.org> - 2017-05-23 07:50 +0200
    Re: [PATCH 7/7] DWARF: add the config option "H. Peter Anvin" <hpa@zytor.com> - 2017-05-19 23:10 +0200

Page 1 of 2  [1] 2  Next page →

#1645923 — Re: [PATCH 7/7] DWARF: add the config option

On Sun, May 07, 2017 at 11:55:24AM -0500, Josh Poimboeuf wrote:
> I'm thinking/hoping that information can be expressed in a simple, easy
> to parse, reasonably sized data structure.  Something like a sorted
> array of this:
> 
> 	struct undwarf {
> 		unsigned int ip;	/* instruction pointer (relative offset from base) */
> 		unsigned prev_frame:13;	/* offset to previous frame from current stack pointer */
> 		unsigned regs:1;	/* whether prev_frame contains entry regs (regs->ip) */
> 		unsigned align:2;	/* some details for dealing with gcc stack realignment */
> 	} __packed;
> 
> 	extern struct undwarf undwarves[];
> 
> One instance of the structure would exist for each time the stack
> pointer changes, e.g. for every function entry, push/pop, and rsp
> add/subtract.  The data could be assembled and sorted offline, possibly
> derived from DWARF, or more likely, generated by objtool.  After doing
> some rough calculations, I think the section size would be comparable to
> the sizes of the DWARF .eh_frame sections it would replace.
> 
> If it worked, the "undwarf" unwinder would be a lot simpler than a real
> DWARF unwinder.  And validating the sanity of the data at runtime would
> be a lot more straightforward.  It could ensure that each stack pointer
> is within the bounds of the current stack, like our current unwinder
> does.

I've been hacking away at this, and so far it's working well.  The code
is much simpler than a DWARF unwinder.  Right now the kernel piece is
only ~350 lines of code.  The vast majority of the changes are in
objtool.

It's now successfully unwinding through entry code and most other asm
files, dumping entry regs, dealing with aligned stacks, dynamic stacks,
etc.

Here's the struct in its current state:

	#define UNDWARF_REG_UNDEFINED		0
	#define UNDWARF_REG_CFA			1
	#define UNDWARF_REG_SP			2
	#define UNDWARF_REG_FP			3
	#define UNDWARF_REG_SP_INDIRECT		4
	#define UNDWARF_REG_FP_INDIRECT		5
	#define UNDWARF_REG_R10			6
	#define UNDWARF_REG_DI			7
	#define UNDWARF_REG_DX			8

	#define UNDWARF_TYPE_NORMAL		0
	#define UNDWARF_TYPE_REGS		1
	#define UNDWARF_TYPE_REGS_IRET		2

	struct undwarf_state {
		int ip;
		unsigned int len;
		short cfa_offset;
		short fp_offset;
		unsigned cfa_reg:4;
		unsigned fp_reg:4;
		unsigned type:2;
	};

With frame pointers disabled, around 300,000 of those structs are needed
for my kernel, which works out to be 4.7M of data.  By comparison, the
DWARF eh_frame sections would be 2.1M.  I think we should be able to
compress it down to a comparable size by rearranging the data a little
bit.

The entry code needs some annotations to give some hints to objtool
about how to generate the data, but it's not bad:

  https://paste.fedoraproject.org/paste/Xq3bPlx5An0Si7AshZTdkF5M1UNdIGYhyRLivL9gydE=

I still have a lot of work to do on the tooling side: module support,
sorting the undwarf table at build time, and a lot of cleanups.  But
overall it's looking feasible.

-- 
Josh

[toc] | [next] | [standalone]

#1645931

On 05/19/17 13:53, Josh Poimboeuf wrote:
> 
> Here's the struct in its current state:
> 
> 	#define UNDWARF_REG_UNDEFINED		0
> 	#define UNDWARF_REG_CFA			1
> 	#define UNDWARF_REG_SP			2
> 	#define UNDWARF_REG_FP			3
> 	#define UNDWARF_REG_SP_INDIRECT		4
> 	#define UNDWARF_REG_FP_INDIRECT		5
> 	#define UNDWARF_REG_R10			6
> 	#define UNDWARF_REG_DI			7
> 	#define UNDWARF_REG_DX			8
> 

Why only those registers?  Also, if you have the option I would really
suggest using the actual x86 register numbers (ax, ex, dx, bx, sp, bp,
si, di, r8-r15 in that order.)

	-hpa

[toc] | [prev] | [next] | [standalone]

#1645961

> How are you handling control flow?

Control flow of what?

> > Here's the struct in its current state:
> > 
> > 	#define UNDWARF_REG_UNDEFINED		0
> > 	#define UNDWARF_REG_CFA			1
> > 	#define UNDWARF_REG_SP			2
> > 	#define UNDWARF_REG_FP			3
> > 	#define UNDWARF_REG_SP_INDIRECT		4
> > 	#define UNDWARF_REG_FP_INDIRECT		5
> > 	#define UNDWARF_REG_R10			6
> > 	#define UNDWARF_REG_DI			7
> > 	#define UNDWARF_REG_DX			8
> > 
> 
> Why only those registers?  Also, if you have the option I would really
> suggest using the actual x86 register numbers (ax, ex, dx, bx, sp, bp,
> si, di, r8-r15 in that order.)

Those are the only registers which are ever needed as the base for
finding the previous stack frame.  99% of the time it's sp or bp, the
other registers are needed for aligned stacks and entry code.

Using the actual register numbers isn't an option because I don't need
them all and they need to fit in a small number of bits.

This construct might be useful for other arches, which is why I called
it "FP" instead of "BP".  But then I ruined that with the last 3 :-)

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1645962

On Fri, May 19, 2017 at 04:29:13PM -0500, Josh Poimboeuf wrote:
> > How are you handling control flow?
> 
> Control flow of what?
> 
> > > Here's the struct in its current state:
> > > 
> > > 	#define UNDWARF_REG_UNDEFINED		0
> > > 	#define UNDWARF_REG_CFA			1
> > > 	#define UNDWARF_REG_SP			2
> > > 	#define UNDWARF_REG_FP			3
> > > 	#define UNDWARF_REG_SP_INDIRECT		4
> > > 	#define UNDWARF_REG_FP_INDIRECT		5
> > > 	#define UNDWARF_REG_R10			6
> > > 	#define UNDWARF_REG_DI			7
> > > 	#define UNDWARF_REG_DX			8
> > > 
> > 
> > Why only those registers?  Also, if you have the option I would really
> > suggest using the actual x86 register numbers (ax, ex, dx, bx, sp, bp,
> > si, di, r8-r15 in that order.)
> 
> Those are the only registers which are ever needed as the base for
> finding the previous stack frame.  99% of the time it's sp or bp, the
> other registers are needed for aligned stacks and entry code.
> 
> Using the actual register numbers isn't an option because I don't need
> them all and they need to fit in a small number of bits.
> 
> This construct might be useful for other arches, which is why I called
> it "FP" instead of "BP".  But then I ruined that with the last 3 :-)

BTW, here's the link to the unwinder code if you're interested:

  https://github.com/jpoimboe/linux/blob/undwarf/arch/x86/kernel/unwind_undwarf.c

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1646054

On Fri, May 19, 2017 at 2:35 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Fri, May 19, 2017 at 04:29:13PM -0500, Josh Poimboeuf wrote:
>> > How are you handling control flow?
>>
>> Control flow of what?
>>
>> > > Here's the struct in its current state:
>> > >
>> > >   #define UNDWARF_REG_UNDEFINED           0
>> > >   #define UNDWARF_REG_CFA                 1
>> > >   #define UNDWARF_REG_SP                  2
>> > >   #define UNDWARF_REG_FP                  3
>> > >   #define UNDWARF_REG_SP_INDIRECT         4
>> > >   #define UNDWARF_REG_FP_INDIRECT         5
>> > >   #define UNDWARF_REG_R10                 6
>> > >   #define UNDWARF_REG_DI                  7
>> > >   #define UNDWARF_REG_DX                  8
>> > >
>> >
>> > Why only those registers?  Also, if you have the option I would really
>> > suggest using the actual x86 register numbers (ax, ex, dx, bx, sp, bp,
>> > si, di, r8-r15 in that order.)
>>
>> Those are the only registers which are ever needed as the base for
>> finding the previous stack frame.  99% of the time it's sp or bp, the
>> other registers are needed for aligned stacks and entry code.
>>
>> Using the actual register numbers isn't an option because I don't need
>> them all and they need to fit in a small number of bits.
>>
>> This construct might be useful for other arches, which is why I called
>> it "FP" instead of "BP".  But then I ruined that with the last 3 :-)
>
> BTW, here's the link to the unwinder code if you're interested:
>
>   https://github.com/jpoimboe/linux/blob/undwarf/arch/x86/kernel/unwind_undwarf.c

At the risk of potentially overcomplicating matters, here's a
suggestion.  As far as I know, all (or most all?) unwinders
effectively do the following in a loop:

1. Look up the IP to figure out how to unwind from that IP.
2. Use the results of that lookup to compute the previous frame state.

The results of step 1 could perhaps be expressed like this:

struct reg_formula {
  unsigned int source_reg :4;
  long offset;
  bool dereference;  /* true: *(reg + offset); false: (reg + offset) */
  /* For DWARF, I think this can be considerably more complicated, but
I doubt it's useful. */
};

struct unwind_step {
  u16 available_regs;  /* mask of the caller frame regs that we are
able to recover */
  struct reg_formula[16];
};

The CFA computation is just reg_formula[UNWIND_REG_SP] (or that plus
or minus sizeof(unsigned long) or whatever -- I can never remember
exactly what CFA refers to.)  For a frame pointer-based unwinder, the
entire unwind_step is a foregone conclusion independent of IP: SP = BP
+ 8 (or whatever), BP = *(BP + whatever), all other regs unknown.

Could it make sense to actually structure the code this way?  I can
see a few advantages.  It would make the actual meat of the unwind
loop totally independent of the unwinding algorithm in use, it would
make the meat be dead simple (and thus easy to verify for
non-crashiness), and I think it just might open the door for a real
in-kernel DWARF unwinder that Linus would be okay with.  Specifically,
write a function like:

bool get_dwarf_step(struct unwind_step *step, unsigned long ip);

Put this function in its own file and make it buildable as kernel code
or as user code.  Write a test case that runs it on every single
address on the kernel image (in user mode!) with address-sanitizer
enabled (or in Valgrind or both) and make sure that (a) it doesn't
blow up and (b) that the results are credible (e.g. by comparing to
objtool output).  Heck, you could even fuzz-test it where the fuzzer
is allowed to corrupt the actual DWARF data.  You could do the same
thing with whatever crazy super-compacted undwarf scheme someone comes
up with down the road, too.

I personally like the idea of using real DWARF annotations in the
entry code because it makes gdb work better (not kgdb -- real gdb
attached to KVM).  I bet that we could get entry asm annotations into
good shape if we really wanted to.  OTOH, getting DWARF to work well
for inline asm is really nasty IIRC.

(H.J., could we get a binutils feature that allows is to do:

pushq %whatever
.cfi_adjust_sp -8
...
popq %whatever
.cfi_adjust_sp 8

that will emit the right DWARF instructions regardless of whether
there's a frame pointer or not?  .cfi_adjust_cfa_offset is not
particularly helpful here because it's totally wrong if the CFA is
currently being computed based on BP.)


Also, you read the stack like this:

*val = *(unsigned long *)addr;

how about probe_kernel_read() instead?

--Andy

[toc] | [prev] | [next] | [standalone]

#1646145

On Fri, May 19, 2017 at 10:23:53PM -0700, Andy Lutomirski wrote:
> On Fri, May 19, 2017 at 2:35 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Fri, May 19, 2017 at 04:29:13PM -0500, Josh Poimboeuf wrote:
> >> > How are you handling control flow?
> >>
> >> Control flow of what?
> >>
> >> > > Here's the struct in its current state:
> >> > >
> >> > >   #define UNDWARF_REG_UNDEFINED           0
> >> > >   #define UNDWARF_REG_CFA                 1
> >> > >   #define UNDWARF_REG_SP                  2
> >> > >   #define UNDWARF_REG_FP                  3
> >> > >   #define UNDWARF_REG_SP_INDIRECT         4
> >> > >   #define UNDWARF_REG_FP_INDIRECT         5
> >> > >   #define UNDWARF_REG_R10                 6
> >> > >   #define UNDWARF_REG_DI                  7
> >> > >   #define UNDWARF_REG_DX                  8
> >> > >
> >> >
> >> > Why only those registers?  Also, if you have the option I would really
> >> > suggest using the actual x86 register numbers (ax, ex, dx, bx, sp, bp,
> >> > si, di, r8-r15 in that order.)
> >>
> >> Those are the only registers which are ever needed as the base for
> >> finding the previous stack frame.  99% of the time it's sp or bp, the
> >> other registers are needed for aligned stacks and entry code.
> >>
> >> Using the actual register numbers isn't an option because I don't need
> >> them all and they need to fit in a small number of bits.
> >>
> >> This construct might be useful for other arches, which is why I called
> >> it "FP" instead of "BP".  But then I ruined that with the last 3 :-)
> >
> > BTW, here's the link to the unwinder code if you're interested:
> >
> >   https://github.com/jpoimboe/linux/blob/undwarf/arch/x86/kernel/unwind_undwarf.c
> 
> At the risk of potentially overcomplicating matters, here's a
> suggestion.  As far as I know, all (or most all?) unwinders
> effectively do the following in a loop:
> 
> 1. Look up the IP to figure out how to unwind from that IP.
> 2. Use the results of that lookup to compute the previous frame state.
> 
> The results of step 1 could perhaps be expressed like this:
> 
> struct reg_formula {
>   unsigned int source_reg :4;
>   long offset;
>   bool dereference;  /* true: *(reg + offset); false: (reg + offset) */
>   /* For DWARF, I think this can be considerably more complicated, but
> I doubt it's useful. */
> };
> 
> struct unwind_step {
>   u16 available_regs;  /* mask of the caller frame regs that we are
> able to recover */
>   struct reg_formula[16];
> };

Ok, so I assume you mean we would need to have an in-kernel DWARF reader
which reads .eh_frame and converts it to the above, which is called for
every step of the unwind phase.

> The CFA computation is just reg_formula[UNWIND_REG_SP] (or that plus
> or minus sizeof(unsigned long) or whatever -- I can never remember
> exactly what CFA refers to.)  For a frame pointer-based unwinder, the
> entire unwind_step is a foregone conclusion independent of IP: SP = BP
> + 8 (or whatever), BP = *(BP + whatever), all other regs unknown.
> 
> Could it make sense to actually structure the code this way?  I can
> see a few advantages.  It would make the actual meat of the unwind
> loop totally independent of the unwinding algorithm in use,

Yes, this part of it is an interesting idea, separating the debuginfo
reading step from the unwinding step.  And for people who don't want to
carry around megs of DWARF data, get_dwarf_step() could just be a fake
lookup which always returns the frame pointer version.  

> it would
> make the meat be dead simple (and thus easy to verify for
> non-crashiness), and I think it just might open the door for a real
> in-kernel DWARF unwinder that Linus would be okay with.  Specifically,
> write a function like:
> 
> bool get_dwarf_step(struct unwind_step *step, unsigned long ip);

If we keep the frame pointer encoding thing for non-DWARF kernels then
we may also need to pass in bp as well.

Or maybe we could force frame pointer users to at least have DWARF data
for the entry code.  Then even frame pointer kernels could detect entry
regs and we could get rid of that nasty frame pointer encoding thing.

> Put this function in its own file and make it buildable as kernel code
> or as user code.  Write a test case that runs it on every single
> address on the kernel image (in user mode!) with address-sanitizer
> enabled (or in Valgrind or both) and make sure that (a) it doesn't
> blow up and (b) that the results are credible (e.g. by comparing to
> objtool output).  Heck, you could even fuzz-test it where the fuzzer
> is allowed to corrupt the actual DWARF data.  You could do the same
> thing with whatever crazy super-compacted undwarf scheme someone comes
> up with down the road, too.

I think your proposal can be separated into two ideas, which can each be
considered on their own merit:

1) Put .eh_frame in the kernel, along with an in-kernel DWARF unwinder.
   Manage the complexity of the unwinder by validating the output of the
   complex part of the algorithm in user space.

   That's a lot of hoops to jump through.  The only real advantage I can
   see is that it would allow us to use the toolchain's DWARF data.  But
   that data is going to have issues anyway because of inline asm,
   special sections (e.g., exception tables), generated code (e.g.,
   bpf), hand-coded asm with missing/incorrect annotations, etc.

   Now, we could use objtool to find such issues and warn about them.
   Then those issues could be corrected, either by hand or
   programmatically.

   But then, if we're going that far, why not just have objtool reformat
   the data into something much simpler?  It already has the knowledge
   to do so.  Then we don't have to jump through all those hoops to
   justify jumping through more hoops in the kernel (i.e., having a
   complex DWARF state machine).  With a simple debuginfo format, the
   kernel unwinder is simple enough that we don't need to validate its
   functionality in a simulator.

2) Make a unified unwinder which uses get_dwarf_step() to abstract out
   the differences between frame pointers and DWARF (or undwarf or
   whatever else).  This could an interesting.  Though I'm not sure how
   it would integrate with our "guess" unwinder for kernels which don't
   have frame pointers or DWARF/undwarf.  I guess we could still keep
   that one separate.

> I personally like the idea of using real DWARF annotations in the
> entry code because it makes gdb work better (not kgdb -- real gdb
> attached to KVM).
>
> I bet that we could get entry asm annotations into
> good shape if we really wanted to.  OTOH, getting DWARF to work well
> for inline asm is really nasty IIRC.
> 
> (H.J., could we get a binutils feature that allows is to do:
> 
> pushq %whatever
> .cfi_adjust_sp -8
> ...
> popq %whatever
> .cfi_adjust_sp 8
> 
> that will emit the right DWARF instructions regardless of whether
> there's a frame pointer or not?  .cfi_adjust_cfa_offset is not
> particularly helpful here because it's totally wrong if the CFA is
> currently being computed based on BP.)

I agree that entry_64.o should have DWARF data, regardless of what the
in-kernel representation of the data looks like.  And the same for all
the other asm .o files.

But how we achieve that is debatable.  In the past, having all the
manual .cfi annotations everywhere for every push/pop was an
unmaintainable disaster.

If we could have binutils do automatic adjustments for pushes and pops
and sp adds/subtracts without the .cfi annotations, that would be great.

Otherwise, objtool can do it.  And it can write both undwarf and DWARF,
if needed.

> Also, you read the stack like this:
> 
> *val = *(unsigned long *)addr;
> 
> how about probe_kernel_read() instead?

It depends on how paranoid you want to be.  The undwarf code has the
level of paranoia the frame pointer code has always had.

In other words, it ensures each dereference is within the bounds of the
current stack.  It relies on task->stack and the percpu exception/irq
stack pointers, as well as the previous stack pointers at the edge of
each stack.

I don't think we've ever had a problem with that level of paranoia, so I
think staying with the status quo might be fine.

If we did use probe_kernel_read() we'd have to use some other form of it
because it does some kasan and hardened usercopy checks which wouldn't
always be appropriate.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1646167

On Sat, May 20, 2017 at 11:20:34AM -0500, Josh Poimboeuf wrote:
>    But then, if we're going that far, why not just have objtool reformat
>    the data into something much simpler?  It already has the knowledge
>    to do so.  Then we don't have to jump through all those hoops to
>    justify jumping through more hoops in the kernel (i.e., having a
>    complex DWARF state machine).  With a simple debuginfo format, the
>    kernel unwinder is simple enough that we don't need to validate its
>    functionality in a simulator.

I should clarify that it doesn't have to be objtool which does this.  It
could instead be a simple DWARF-to-undwarf conversion tool which runs
during the vmlinux linking stage.

Anyway we're both proposing simplifying the DWARF data into an
easier-to-parse format.  I think the question is whether we want that
simplification process to happen in the kernel (in the middle of a
kernel unwind operation), or at build time.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1646205

On Sat, May 20, 2017 at 9:20 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:

>>
>> (H.J., could we get a binutils feature that allows is to do:
>>
>> pushq %whatever
>> .cfi_adjust_sp -8
>> ...
>> popq %whatever
>> .cfi_adjust_sp 8
>>

Np.  Compiler needs to generate this.

-- 
H.J.

[toc] | [prev] | [next] | [standalone]

#1646215

On Sat, May 20, 2017 at 1:01 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Sat, May 20, 2017 at 9:20 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
>>>
>>> (H.J., could we get a binutils feature that allows is to do:
>>>
>>> pushq %whatever
>>> .cfi_adjust_sp -8
>>> ...
>>> popq %whatever
>>> .cfi_adjust_sp 8
>>>
>
> Np.  Compiler needs to generate this.
>

How would the compiler generate this when inline asm is involved?  For
the kernel, objtool could get around the need to have these
annotations, but not so much for user code?  Is the compiler supposed
to parse the inline asm?  Would the compiler provide some magic % code
to represent the current CFA base register?

[toc] | [prev] | [next] | [standalone]

#1646218

On Sat, May 20, 2017 at 2:58 PM, Andy Lutomirski <luto@kernel.org> wrote:
> On Sat, May 20, 2017 at 1:01 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Sat, May 20, 2017 at 9:20 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>>
>>>>
>>>> (H.J., could we get a binutils feature that allows is to do:
>>>>
>>>> pushq %whatever
>>>> .cfi_adjust_sp -8
>>>> ...
>>>> popq %whatever
>>>> .cfi_adjust_sp 8
>>>>
>>
>> Np.  Compiler needs to generate this.
>>
>
> How would the compiler generate this when inline asm is involved?  For
> the kernel, objtool could get around the need to have these
> annotations, but not so much for user code?  Is the compiler supposed
> to parse the inline asm?  Would the compiler provide some magic % code
> to represent the current CFA base register?

Here is one example of inline asm with call frame info:

https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/x86_64/sigaction.c;h=be058bac436d1cc9794b2b03107676ed99f6b872;hb=HEAD

-- 
H.J.

[toc] | [prev] | [next] | [standalone]

#1646769

On Sat, 20 May 2017, H.J. Lu wrote:

> >>>> pushq %whatever
> >>>> .cfi_adjust_sp -8
> >>>> ...
> >>>> popq %whatever
> >>>> .cfi_adjust_sp 8
> >>>>
> >>
> >> Np.  Compiler needs to generate this.
> >>
> >
> > How would the compiler generate this when inline asm is involved?  For
> > the kernel, objtool could get around the need to have these
> > annotations, but not so much for user code?  Is the compiler supposed
> > to parse the inline asm?  Would the compiler provide some magic % code
> > to represent the current CFA base register?
> 
> Here is one example of inline asm with call frame info:
> 
> https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/x86_64/sigaction.c;h=be058bac436d1cc9794b2b03107676ed99f6b872;hb=HEAD

That brings us basically pretty close to square one though; having to 
maintain "manual" anotations. Something we're pretty much trying to avoid 
through this excercise.

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1647001

On Mon, May 22, 2017 at 4:34 AM, Jiri Kosina <jikos@kernel.org> wrote:
> On Sat, 20 May 2017, H.J. Lu wrote:
>
>> >>>> pushq %whatever
>> >>>> .cfi_adjust_sp -8
>> >>>> ...
>> >>>> popq %whatever
>> >>>> .cfi_adjust_sp 8
>> >>>>
>> >>
>> >> Np.  Compiler needs to generate this.
>> >>
>> >
>> > How would the compiler generate this when inline asm is involved?  For
>> > the kernel, objtool could get around the need to have these
>> > annotations, but not so much for user code?  Is the compiler supposed
>> > to parse the inline asm?  Would the compiler provide some magic % code
>> > to represent the current CFA base register?
>>
>> Here is one example of inline asm with call frame info:
>>
>> https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/x86_64/sigaction.c;h=be058bac436d1cc9794b2b03107676ed99f6b872;hb=HEAD
>
> That brings us basically pretty close to square one though; having to
> maintain "manual" anotations. Something we're pretty much trying to avoid
> through this excercise.

Assembler only encodes instructions.  You need to a different tool
to figure out what an instruction does.


-- 
H.J.

[toc] | [prev] | [next] | [standalone]

#1647406

On 05/20/17 13:01, H.J. Lu wrote:
> On Sat, May 20, 2017 at 9:20 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
>>>
>>> (H.J., could we get a binutils feature that allows is to do:
>>>
>>> pushq %whatever
>>> .cfi_adjust_sp -8
>>> ...
>>> popq %whatever
>>> .cfi_adjust_sp 8
>>>
> 
> Np.  Compiler needs to generate this.
> 

For actual assembly we have such a feature, it is called macros.

push/pop is the easy stuff; macros take care of that, but the real pain
is dealing with the flow of control.

	-hpa

[toc] | [prev] | [next] | [standalone]

#1647423

On 05/22/17 14:07, H. Peter Anvin wrote:
> On 05/20/17 13:01, H.J. Lu wrote:
>> On Sat, May 20, 2017 at 9:20 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>>
>>>>
>>>> (H.J., could we get a binutils feature that allows is to do:
>>>>
>>>> pushq %whatever
>>>> .cfi_adjust_sp -8
>>>> ...
>>>> popq %whatever
>>>> .cfi_adjust_sp 8
>>>>
>>
>> Np.  Compiler needs to generate this.
>>
> 
> For actual assembly we have such a feature, it is called macros.
> 
> push/pop is the easy stuff; macros take care of that, but the real pain
> is dealing with the flow of control.
> 

My biggest beef with the CFI directives that gas uses is that there is
that .cfi_remember_state/.cfi_restore_state doesn't have a way to
specify more than one state.  That makes it really hard to get sanity
around control flow changes, especially with code that is intentionally
out of line.

That, and some of the CFI directives seem to be a bit ill-defined in
their definition (are they even applicable to anything other than
DWARF?)  They almost seem to be referencing some external specification,
but the only thing I'm finding is the DWARF documentation which is
written in very different terms.

The best description of what a personality routine is I found in an
article by Ian Lance Taylor.  It doesn't seem to be applicable to C as
far as I can tell.

	-hpa

[toc] | [prev] | [next] | [standalone]

#1647440

On Mon, May 22, 2017 at 02:37:50PM -0700, H. Peter Anvin wrote:
> On 05/22/17 14:07, H. Peter Anvin wrote:
> > On 05/20/17 13:01, H.J. Lu wrote:
> >> On Sat, May 20, 2017 at 9:20 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >>
> >>>>
> >>>> (H.J., could we get a binutils feature that allows is to do:
> >>>>
> >>>> pushq %whatever
> >>>> .cfi_adjust_sp -8
> >>>> ...
> >>>> popq %whatever
> >>>> .cfi_adjust_sp 8
> >>>>
> >>
> >> Np.  Compiler needs to generate this.
> >>
> > 
> > For actual assembly we have such a feature, it is called macros.
> > 
> > push/pop is the easy stuff; macros take care of that, but the real pain
> > is dealing with the flow of control.
> > 
> 
> My biggest beef with the CFI directives that gas uses is that there is
> that .cfi_remember_state/.cfi_restore_state doesn't have a way to
> specify more than one state.  That makes it really hard to get sanity
> around control flow changes, especially with code that is intentionally
> out of line.
> 
> That, and some of the CFI directives seem to be a bit ill-defined in
> their definition (are they even applicable to anything other than
> DWARF?)  They almost seem to be referencing some external specification,
> but the only thing I'm finding is the DWARF documentation which is
> written in very different terms.
> 
> The best description of what a personality routine is I found in an
> article by Ian Lance Taylor.  It doesn't seem to be applicable to C as
> far as I can tell.

So my understanding is that there's stock DWARF (.debug_frame) and then
there's souped-up DWARF (.eh_frame), which is basically DWARF with a few
extensions.

The remember/restore state thing is stock DWARF (DW_CFA_remember_state
and DW_CFA_restore_state).  The personality routine thing is in the
.eh_frame extension which is documented here:

  http://refspecs.linuxfoundation.org/LSB_5.0.0/LSB-Core-generic/LSB-Core-generic/ehframechpt.html

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1646211

On Fri, May 19, 2017 at 10:23 PM, Andy Lutomirski <luto@kernel.org> wrote:
>
> I personally like the idea of using real DWARF annotations in the
> entry code because it makes gdb work better (not kgdb -- real gdb
> attached to KVM).  I bet that we could get entry asm annotations into
> good shape if we really wanted to.  OTOH, getting DWARF to work well
> for inline asm is really nasty IIRC.

No. I will NAK *any* attempt to make our asm contain the crazy
shit-for-brains annotations.

Been there, done that, got the T-shirt, and then doused the T-shirt in
gasoline and put it on fire.

The amount of unreadable crap and bugs it requires is not worth the
pain. Not for *any* amount of gain, and the gain here is basically
zero.

> (H.J., could we get a binutils feature that allows is to do:
>
> pushq %whatever
> .cfi_adjust_sp -8
> ...
> popq %whatever
> .cfi_adjust_sp 8
>
> that will emit the right DWARF instructions regardless of whether
> there's a frame pointer or not?  .cfi_adjust_cfa_offset is not
> particularly helpful here because it's totally wrong if the CFA is
> currently being computed based on BP.)

Yeah, that's just a small example of the kind of crap people have to deal with.

Not going to happen. Assembler files are hard enough to write (and
read) as-is, anything that expects us to go back to the bad old days
with crazy shit cfi annotations is going to get violently NAK'ed and
vetoed forever.

The *only* acceptable model is automated tools (ie objtool). Don't
even bother to try to go any other way. Because I will not accept that
shit.

                 Linus

[toc] | [prev] | [next] | [standalone]

#1646216

On Sat, May 20, 2017 at 1:16 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, May 19, 2017 at 10:23 PM, Andy Lutomirski <luto@kernel.org> wrote:
>>
>> I personally like the idea of using real DWARF annotations in the
>> entry code because it makes gdb work better (not kgdb -- real gdb
>> attached to KVM).  I bet that we could get entry asm annotations into
>> good shape if we really wanted to.  OTOH, getting DWARF to work well
>> for inline asm is really nasty IIRC.
>
> No. I will NAK *any* attempt to make our asm contain the crazy
> shit-for-brains annotations.
>
> Been there, done that, got the T-shirt, and then doused the T-shirt in
> gasoline and put it on fire.
>
> The amount of unreadable crap and bugs it requires is not worth the
> pain. Not for *any* amount of gain, and the gain here is basically
> zero.

But what if objtool autogenerated the annotations, perhaps with a tiny
bit of help telling it "hardware frame goes here" or "pt_regs goes
here"?

[toc] | [prev] | [next] | [standalone]

#1646221

On Sat, May 20, 2017 at 2:56 PM, Andy Lutomirski <luto@kernel.org> wrote:
> On Sat, May 20, 2017 at 1:16 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> The amount of unreadable crap and bugs it requires is not worth the
>> pain. Not for *any* amount of gain, and the gain here is basically
>> zero.
>
> But what if objtool autogenerated the annotations, perhaps with a tiny
> bit of help telling it "hardware frame goes here" or "pt_regs goes
> here"?

You snipped the next part of my email, where I said:

> The *only* acceptable model is automated tools (ie objtool). Don't
> even bother to try to go any other way. Because I will not accept that
> shit.

so yes, objtool parsing things on its own is acceptable (and it had
better not need any help - it already checks frame pointer data).

The CFI annotations needed in asm are horrendous. We used to have
them, and we didn't have even _remotely_ complete annotations and
despite that they were
 (a) wrong
 (b) incomplete
 (c) made the asm impossible to read and even worse to modify.

hjl already posted an example of the kinds of horrors glibc does to do
things "right". And those rabbit ears around "right" are there for a
reason. There's no way that is ever right - even if it gets the right
results, it's an unmaintainable piece of crap.

So no, we're never ever adding that CFI garbage back into the kernel.

A tool that can generate it is ok, but even then we should expect
inevitable bugs and not trust the end result blindly.

Because dwarf info is complex enough that other tools have gotten it
wrong many many times. Just google for "gcc bugzilla cfi" or go to the
gcc bugzilla and search for "DWARF" or whatever. It's not "oh, we once
had a bug". It's constant.

One of the reasons I like the idea of having objtool generate
something *simpler* than dwarf is that it not only is much easier to
parse, it has a much higher likelihood of not having some crazy bug.
If objtool mainly looks at the actual instructions, and perhaps uses
dwarf information as additional input and creates something much
simpler than dwarf, it might have a chance in hell of occasionally
even getting it right.

Because dwarf information is really really complicated. It's
complicated because it contains *way* more information than just how
to find the next stack frame.

I mean, it has basically a RPN interpreter built in, and that's the
_simple_ part.

                  Linus

[toc] | [prev] | [next] | [standalone]

#1646222

On Sat, May 20, 2017 at 4:00 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> hjl already posted an example of the kinds of horrors glibc does to do
> things "right".

Side note: we'd hopefully/presumably never need anything _that_
disgusting for the kernel, so hjl's example is probably an extreme
one.

But even when we just did the pushq/popq_cfi macros etc to try to have
simple and reasonably legible annotations for the common cases, it got
pretty ugly.

It wasn't that extreme glibc kind of "50 lines of ugly for two
instructions of code", but it was pretty bad. And as far as I know we
never even tried to annotate places where we did "pushf/pop %reg" in
inline asm (for saving/restoring flags)

               Linus

[toc] | [prev] | [next] | [standalone]

#1646754

* Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> > How are you handling control flow?
> 
> Control flow of what?
> 
> > > Here's the struct in its current state:
> > > 
> > > 	#define UNDWARF_REG_UNDEFINED		0
> > > 	#define UNDWARF_REG_CFA			1
> > > 	#define UNDWARF_REG_SP			2
> > > 	#define UNDWARF_REG_FP			3
> > > 	#define UNDWARF_REG_SP_INDIRECT		4
> > > 	#define UNDWARF_REG_FP_INDIRECT		5
> > > 	#define UNDWARF_REG_R10			6
> > > 	#define UNDWARF_REG_DI			7
> > > 	#define UNDWARF_REG_DX			8
> > > 
> > 
> > Why only those registers?  Also, if you have the option I would really
> > suggest using the actual x86 register numbers (ax, ex, dx, bx, sp, bp,
> > si, di, r8-r15 in that order.)
> 
> Those are the only registers which are ever needed as the base for
> finding the previous stack frame.  99% of the time it's sp or bp, the
> other registers are needed for aligned stacks and entry code.
> 
> Using the actual register numbers isn't an option because I don't need
> them all and they need to fit in a small number of bits.
> 
> This construct might be useful for other arches, which is why I called
> it "FP" instead of "BP".  But then I ruined that with the last 3 :-)

Please call it BP - 'FP' can easily be read as floating-point, making it all 
super-confusing. We should use canonical x86 register names and ordering - even
if not all registers are used straight away.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web