Re: [PATCH v2 02/14] objtool: Fix ORC vs alternatives

[Miroslav Benes <mbenes@suse.cz>]

On Wed, 29 Apr 2020, Peter Zijlstra wrote:

> On Wed, Apr 29, 2020 at 04:33:31PM +0200, Miroslav Benes wrote:
> > On Tue, 28 Apr 2020, Peter Zijlstra wrote:
> > >  /*
> > > + * Alternatives should not contain any ORC entries, this in turn means they
> > > + * should not contain any CFI ops, which implies all instructions should have
> > > + * the same same CFI state.
> > > + *
> > > + * It is possible to constuct alternatives that have unreachable holes that go
> > > + * unreported (because they're NOPs), such holes would result in CFI_UNDEFINED
> > > + * states which then results in ORC entries, which we just said we didn't want.
> > > + *
> > > + * Avoid them by copying the CFI entry of the first instruction into the whole
> > > + * alternative.
> > > + */
> > > +static void fill_alternative_cfi(struct objtool_file *file, struct instruction *insn)
> > > +{
> > > +	struct instruction *first_insn = insn;
> > > +	int alt_group = insn->alt_group;
> > > +
> > > +	sec_for_each_insn_continue(file, insn) {
> > > +		if (insn->alt_group != alt_group)
> > > +			break;
> > > +		insn->cfi = first_insn->cfi;
> > > +	}
> > > +}
> > 
> > If I am reading this and previous patch correctly...
> > 
> > The function would copy cfi only to "orig" alternative (its insn->alts is 
> > non-empty, orig_insn->alt_group differs from new_insn->alt_group), right? 
> 
> Yes.
> 
> > Would it make sense to do the same for "new" alternative, because of the 
> > invariant? It seems to me it is not processed anywhere that way.
> 
> No.
> 
> > Am I missing something? Whenever I try to read all this alternatives 
> > handling in objtool, I get lost pretty soon.
> 
> We only care about the ORC covering the original range, because that is
> the range we execute code from. The memory where we store the
> alternative instructions (.altinstruction section) is never executed,
> that is, RIP should never point there, so we don't need ORC data covering
> it.

Aha, that's what I didn't realize (again). Note to myself (for the 
hundredth time): alternatives are not branches.

Thanks
Miroslav