[PATCH] covoar: Add option to create named objdumps

[Alex White]

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.

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.

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.

>
> >> -        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.

> >
> > 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 behavior is unchanged.

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

None that I have found.

> >
> > 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?

>
> Chris
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel