[PATCH] covoar: fixing the extension mismatch in trace file

[Vijay Kumar Banerjee]

On 13 May 2018 at 10:50, Chris Johns <chrisj at rtems.org> wrote:

> On 13/5/18 4:44 pm, Vijay Kumar Banerjee wrote:
> > On Sun, 13 May 2018, 10:09 Chris Johns, <chrisj at rtems.org
> > <mailto:chrisj at rtems.org>> wrote:
> >     >
> >     >        Coverage::CoverageFormats_t   coverageFormat =
> >     >     Coverage::COVERAGE_FORMAT_QEMU;
> >     >        Coverage::CoverageReaderBase* coverageReader = NULL;
> >     >        char*                         executable = NULL;
> >     >     @@ -317,11 +317,12 @@ int main(
> >     >              std::cerr << "warning: Unable to read executable: "
> << argv[i] <<
> >     >     std::endl;
> >     >            } else {
> >     >              coverageFileName = argv[i];
> >     >     -        coverageFileName.replace(
> >     >     +       coverageFileName.append(coverageExtension);
> >     >     +     /* coverageFileName.replace(
> >     >                coverageFileName.length() -
> executableExtension.size(),
> >     >                executableExtension.size(),
> >     >                coverageExtension
> >     >     -        );
> >     >     +        ); */
> >     >
> >     >
> >     >
> >     > If you are replacing this call, then just delete it -- don't
> comment it out.
> >     >
> >
> >     I suggest the replace be changed to move past the '.' in the file
> name.
> >
> >     I suspect the code here is still fragile because the extensions need
> to be the
> >     same size.
> >
> > can't this be done by adding '.' in the append as in the v2 of this
> patch to
> > keep the extension size same  ?
>
> It fixes something but what is broken that is being fixed?
>
> the trace files have .exe.cov extensions but the covoar was searching for
.cov extension causing it to return error for not finding the trace file.
Adding the .cov to the coverageFileName to make it .exe.cov seemed like a
quick fix to this .

Do we need the .exe extension there ?

> are you suggesting to use replace instead ?
>
> I think it would be better to fix the bug than work around it.
>
> The replace is saying replace from length of the extension back from the
> end of
> the string for the size of the extension with the extension. In other
> words it
> should back up from the end the extension length and replace it. Which
> length or
> size is wrong.
>
> No matter what the current code is fragile because it assumes a few
> things. The
> most robust approach would add code to the rtemstoolkit's rld::path
> namespace to
> return a path with the extension stripped. For example add
> 'rld::path::strip_extension ()' and then use that to strip
> coverageFileName and
> then add the extension.
>
> I would also review the 'exe' extension usage to make sure it is OK. I
> have not
> done this.
>
> Are you interested in doing this?
>

I can look into this. Any instructions ?

>
> Chris
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20180513/ed6f2896/attachment-0002.html>