On Tue, Feb 08, 2022 at 10:22:06AM +0530, Vimal Agrawal wrote:

quoted

quoted

Actually I had it  under (!best) in my first patch. I had to change it
because it was resolving the address to symbols like __this_module for
init address at times which is not correct for text address.

Can you say that again?

I hit this a few times later after the first patch. Basically there
are all symbols in symbol table and best could be none zero ( means it
matched some symbol) but it may not be match to .text symbol for text
address ( esp. when --strip-unneeded is used as there are very few
symbols left after stripping)

You are saying that sometimes having "best" evaluated to non zero
yields incorrect results, where the symbol found is actualy not a .text
symbol for a text address? If so, is this really true for cases where
no stripping is used? If so this is bigger news and I'd like to address
this separately in another commit but we need proof, not just
speculation.

And you seem to be suggesting that this seems to hold more true when
"--strip-unneeded" is used given there are fewer symbols left after
striping?

Did I comprehend what you are trying to say correctly?

quoted

OK so you're saying sometimes "best" is not true when we use
INSTALL_MOD_STRIP="--strip-unneeded"? This is news.

yes, best can be non zero and may not resolve to .text address when
--strip-unneeded is used. 

OK.

without stripping, it will definitely
resolve to some .text address closely matching in case of no stripping

OK so there is no issue when stripping is used.

but it can go wrong with stripping. I have seen it a few times post
the first patch during testing.

OK then we need to take care your added heuristics do not affect
non-stripping.

quoted

In particulr you seem to be suggesting that if --strip-unneeded was
used "best" could be incorrect for !is_module_text_address().

best could be incorrect even for text address when --strip-unneeded is used.
e.g. in my case, it is resolving .init.text address to __this_module

You should be explicit about this in your commit log.

quoted

In any case, you completely changed things in your patch and did not
mention *any* of this in your follow up patch, leaving me to question
the validity of all this work.

The Only change I did from the first patch was to move this hunk out of (!best).
Yes, I missed commenting it in the code.

When you submit a v2 patch and you change something like that you must
clarify changes which are not clear either in the commit log or below
the --- lines after the diffstat and before the actual patch. Each new
patch iteration should have a set of bullets with all the changes you
have made so that the maintainer can track what you have done
differently on each iteration.

Right now you are not making any of this easy on me so I ask that you
stop submitting new patches willy nilly until we have actualy discussed
each item, and we decide what to do. I also ask that you keep track of
each change you are making on each new patch iteration on the patch
after the --- lines and before the patch, so I can easily tell all the
changes you have made on each new iteration.

  Luis