Re: [PATCH v2 01/12] objtool: Generic annotation infrastructure

[Peter Zijlstra <peterz@infradead.org>]

On Fri, Nov 15, 2024 at 10:38:28AM -0800, Josh Poimboeuf wrote:
> On Mon, Nov 11, 2024 at 12:59:36PM +0100, Peter Zijlstra wrote:
> > +#define ASM_ANNOTATE(x)						\
> > +	"911:\n\t"						\
> > +	".pushsection .discard.annotate,\"M\",@progbits,8\n\t"	\
> > +	".long 911b - .\n\t"					\
> > +	".long " __stringify(x) "\n\t"				\
> > +	".popsection\n\t"
> 
> Why mergeable and progbits?

In order to get sh_entsize ?

> > +static int read_annotate(struct objtool_file *file, void (*func)(int type, struct instruction *insn))
> > +{
> > +	struct section *rsec, *sec;
> > +	struct instruction *insn;
> > +	struct reloc *reloc;
> > +	int type;
> > +
> > +	rsec = find_section_by_name(file->elf, ".rela.discard.annotate");
> > +	if (!rsec)
> > +		return 0;
> > +
> > +	sec = find_section_by_name(file->elf, ".discard.annotate");
> > +	if (!sec)
> > +		return 0;
> 
> Instead of looking for .rela.discard.annotate you can just get it from
> sec->rsec.

Oh, indeed.

> > +
> > +	if (sec->sh.sh_entsize != 8) {
> > +		static bool warn = false;
> 
> "warned" ?

Sure.

> > +		if (!warn) {
> > +			WARN("%s: dodgy linker, sh_entsize != 8", sec->name);
> > +			warn = true;
> > +		}
> 
> Any reason not to make this a fatal error?

lld is currently suffering from this, it would get us build failures on
llvm builds. Once that's fixed, then yes, this should become fatal.

  https://github.com/ClangBuiltLinux/linux/issues/2057

> > +		sec->sh.sh_entsize = 8;
> > +	}
> > +
> > +	for_each_reloc(rsec, reloc) {
> > +		insn = find_insn(file, reloc->sym->sec,
> > +				 reloc->sym->offset + reloc_addend(reloc));
> > +		if (!insn) {
> > +			WARN("bad .discard.annotate entry: %d", reloc_idx(reloc));
> > +			return -1;
> > +		}
> 
> Would be nice to print the type here as well.

Sure.

> > @@ -2670,6 +2714,8 @@ static int decode_sections(struct objtoo
> >  	if (ret)
> >  		return ret;
> >  
> > +	ret = read_annotate(file, __annotate_nop);
> > +
> 
> 'ret' is ignored here (not that it matters much as this goes away in the
> next patch)

Right..