[PATCH] covoar: Add option to create named objdumps

[Chris Johns]

On 30/3/21 9:10 am, Alex White wrote:
> On Mon, Mar 29, 2021 at 4:39 PM Chris Johns <chrisj at rtems.org> wrote:
>>
>> On 30/3/21 7:56 am, Gedare Bloom wrote:
>>> On Fri, Mar 12, 2021 at 10:07 AM Alex White <alex.white at oarcorp.com> wrote:
>>>>
>>>> This adds a new behavior to the -d option which allows the creation of
>>>> named objdump outputs in the /tmp directory. This allows the outputs to
>>>> be reused on subsequent runs of covoar rather than running objdump
>>>> again.
>>> This seems to be a hackish way to optimize a behavior. Do you guys
>>> have a plan to improve this caching mechanism?
>>
>> Agreed. If objdump and generated files is being used then caching and management
>> across instances of covoar will be complex but that is one of the trade-offs
>> when wrapping objdump.
> 
> At the moment, there is no plan to improve it, no.
> 
> The original behavior of -d was just to keep the temp files around after a run.
> This change augments that behavior to also give the temp files useful names. It
> allows for a nice speedup when trying to do multiple runs for debugging
> purposes since the temp files can now be checked to see if they need to be
> regenerated.

This is different to improving performance and that is what the commit comments
say. And changing the commit comments now to discuss a debug option that is
there for performance testing only makes life harder for us as reviewers.

> Ideally, yes, this would be more sophisticated and less hackish. I hid it behind
> the debug flag since it was added as a means to test my changes more quickly
> rather than as a fully-fledged file caching feature.

Great and so it seems the test has worked. There is no need to merge the test
code. A full solution would be welcome.

> If the caching mechanism were improved, I would likely move it to its own flag.
> That would allow the speedup to be realized on more than just debug runs.

Great, sounds like a plan.

>>>> -        throw rld::error( "Objdump error", "generating objdump" );
>>>> +      if (!debug || FileIsNewer( fileName.c_str(), objdumpFile.name().c_str() )) {
>>> what's this !debug
> 
> `debug` is set at the top level to `true` if the "-d" option is provided, false
> otherwise.

Yes I know. I changed that top level to be more C++ 3 years ago unfunded to try
and provide a pathway for more changes to happen.

>>> Relying on timestamps can be tricky. You all should consider how to
>>> create a proper cache of files, probably in the local directory from
>>> which this tool is run, that updates based on hashed contents of the
>>> source files rather than timestamps.  This is for your TODO list.
>>
>> The commit comment says the files are in /tmp. Using /tmp to hold files that are
>> not temporary seems the contradict the idea of a temporary directory.
>>
>> Who cleans up the files in /tmp that are left there?
> 
> The original behavior of the "-d" option was to preserve the generated files in the /tmp directory, I believe.

This is the part that is disconnected and does not match. I am concerned it
becomes a back door performance hack. Some systems use RAM disks for /tmp for
performance so having it fill would be a problem.

> This behavior is unchanged.

I think it is changed with this patch.

>>>> +{
>>>> +  size_t idx = path.rfind('/', path.length());
>>> Are there any library helpers that can do this?
> 
> None that I have found.

Hmmmm ....

https://git.rtems.org/rtems-tools/tree/rtemstoolkit/rld-path.h#n44

This is not the first tool in our tool suite that has had to deal with this.

>>>
>>> What happens if you run this on windows? (I know the answer, this
>>> stuff doesn't work on windows. But it doesn't mean we shouldn't
>>> consider cross-platform issues?)
>>
>> Windows is a supported host for this project so I hope it is tested on Windows.
>> Not building or not working just means someone else needs to fix the problem.
> 
> I was only running with `-d` on Linux and FreeBSD so I wasn't thinking about it.
> 
> Should I change it to handle separators on Windows using an ifdef?

No. I suggest to stop guessing and testing our patience.

Chris