Re: [PATCH v3 04/13] recordmcount: Rewrite error/success handling

[Matt Helsley <mhelsley@vmware.com>]

> On Jul 26, 2019, at 10:45 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> On Wed, 24 Jul 2019 14:04:58 -0700
> Matt Helsley <mhelsley@vmware.com> wrote:
> 
> 
> Hi Matt,
> 
> As I'm applying these for real, I'm taking a deeper look at the
> patches. This one I have some questions about.
> 
>> Recordmcount uses setjmp/longjmp to manage control flow as
>> it reads and then writes the ELF file. This unusual control
>> flow is hard to follow and check in addition to being unlike
>> kernel coding style.
>> 
>> So we rewrite these paths to use regular return values to
>> indicate error/success. When an error or previously-completed object
>> file is found we return an error code following kernel
>> coding conventions -- negative error values and 0 for success when
>> we're not returning a pointer. We return NULL for those that fail
>> and return non-NULL pointers otherwise.
>> 
>> One oddity is already_has_rel_mcount -- there we use pointer comparison
>> rather than string comparison to differentiate between
>> previously-processed object files and returning the name of a text
>> section.
> 
> This is fine, but it's got a strange implementation.
> 
> 
> 
>> diff --git a/scripts/recordmcount.h b/scripts/recordmcount.h
>> index c1e1b04b4871..909a3e4775c2 100644
>> --- a/scripts/recordmcount.h
>> +++ b/scripts/recordmcount.h
>> @@ -24,7 +24,9 @@
>> #undef mcount_adjust
>> #undef sift_rel_mcount
>> #undef nop_mcount
>> +#undef missing_sym
>> #undef find_secsym_ndx
>> +#undef already_has_rel_mcount
> 
> Why do we need these as defines? Can't you just have a single:
> 
> const char *already_has_mcount = "success";
> 
> in recordmcount.c before recordmcount.h is included?
> 
> And same for missing_sym.

Yes, that’s a good point. I’ve been trying to separate the changes to the functions
from moving parts out but in this case it would make just as much sense to add them to recordmcount.c in the first place.

Ultimately, this ugliness gets removed as the next series removes recordmcount.h entirely and one of the steps is moving find_secsym_ndx() out while eliminating these redundant pieces.

> 
> Another, probably more robust way of doing this, is change
> find_secsym_ndx() to return 0 on success and -1 on missing symbol, and
> just pass a pointer by reference to fill the recsym (which doesn't have
> to be a constant).

That’s easy enough to do  and  I do like separating the error/success return from returning  the index. I can send that out now or tack it onto the next RFC series I’m about to send which completes the conversion if that’s preferable.

Yeah, the original code applies “const” in lots of places -- I presume it’s an attempt to eek out every last bit of performance from the compiler.

Cheers,
     -Matt