Re: [PATCH] pahole: do not return an error when printing only a specific class

[Ihor Solodrai <ihor.solodrai@linux.dev>]

On 7/23/25 3:03 AM, Alan Maguire wrote:
> On 23/07/2025 09:39, Alexis Lothoré wrote:
>> Hi Ihor,
>>
>> On Wed Jul 23, 2025 at 1:42 AM CEST, Ihor Solodrai wrote:
>>> On 7/16/25 2:04 AM, Alexis Lothoré (eBPF Foundation) wrote:
>>
>> [...]
>>
>>>>    	cu__finalize(cu, cus, conf);
>>>> -	if (cus__steal_now(cus, cu, conf) == LSK__STOP_LOADING)
>>>> +	if (cus__steal_now(cus, cu, conf) == LSK__ABORT)
>>>>    		goto out_abort;
>>>
>>> I think we should handle both LSK__STOP_LOADING and LSK__ABORT here,
>>> otherwise the worker will keep loading DWARF (which is most of the
>>> pahole's work) and passing cu-s to the stealer function.
>>>
>>> I guess in many cases it'll complete without errors, but it's just
>>> unnecessary work.
>>>
>>> Maybe add a check for LSK__STOP_LOADING here with goto_ok?
>>
>> Hmmm, I am not sure how to follow you here. Even if cus__steal_now returns
>> LSK__STOP_LOADING, I need to return DWARF_CB_OK from
>> cus__merge_and_process_cu, otherwise it will be processed as an error
>> again. Or are you suggesting just make sure to delete current cu but still
>> return DWARF_CB_OK when getting LSK__STOP_LOADING, as done below ?

LSK__STOP_LOADING signals that no more input needs to be processed.

So I was thinking we should make sure the workers terminate without
error and return DWARF_CB_OK. Whether we delete a cu or not is not
that important since we are exiting soon anyway.

>>
>> --- a/dwarf_loader.c
>> +++ b/dwarf_loader.c
>> @@ -3796,6 +3796,7 @@ static int cus__merge_and_process_cu(struct cus *cus, struct conf_load *conf,
>>          Dwarf_Off off = 0, noff;
>>          struct cu *cu = NULL;
>>          size_t cuhl;
>> +       int res;
>>
>>          while (dwarf_nextcu(dw, off, &noff, &cuhl, NULL, &pointer_size,
>>                              &offset_size) == 0) {
>> @@ -3874,7 +3875,8 @@ static int cus__merge_and_process_cu(struct cus *cus, struct conf_load *conf,
>>                  goto out_abort;
>>
>>          cu__finalize(cu, cus, conf);
>> -       if (cus__steal_now(cus, cu, conf) == LSK__ABORT)
>> +       res = cus__steal_now(cus, cu, conf);
>> +       if (res == LSK__STOP_LOADING || res == LSK__ABORT)
>>                  goto out_abort;
>>
>>          return 0;
>> @@ -3882,7 +3884,7 @@ static int cus__merge_and_process_cu(struct cus *cus, struct conf_load *conf,
>>   out_abort:
>>          dwarf_cu__delete(cu);
>>          cu__delete(cu);
>> -       return DWARF_CB_ABORT;
>> +       return res == LSK__STOP_LOADING ? DWARF_CB_OK : DWARF_CB_ABORT;
>>   }
>>
>>
>> Thanks,
>>
>> Alexis
>>
>>
> 
> Good point; I think ideally we'd like to stop processing in either case
> but still distinguish error from early exit. We could perhaps set the
> appropriate LSK_ value in dcus (dcus->lsk_status?) to make the
> distinction and use it in cus__load_module(), renaming out_error in
> dwarf_cus__process_cu()  to out_early or something to underline the fact
> that it's an early exit not necessarily a failure. So we preserve the
> return of CB_ABORT for both failure and early exit, but can still tell
> these apart using dcus->lsk_status. Would that work?

Good suggestion. Yes, I think that would work better than just return
DWARF_CB_OK from the worker.

I overlooked the fact that there are many workers, and one of them
exiting with DWARF_CB_OK would not affect the others. But
DWARF_CB_ABORT would via the cus_processing_queue.abort flag.