maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Anel!

On Oct 30, Anel Husakovic wrote:
> revision-id: f112c4a07475 (mariadb-10.2.31-541-gf112c4a07475)
> parent(s): 784473b98662
> author: Anel Husakovic <anel@xxxxxxxxxxx>
> committer: Anel Husakovic <anel@xxxxxxxxxxx>
> timestamp: 2020-10-26 14:25:41 +0100
> message:
> 
> MDEV-20787: Script dgcov.pl does not work
> 
> - Patch is changing CMakeList with `--coverage` flag as an alias for
> `-fprofile-arcs -ftest-coverage -lgcov` in addition.
> 
> - About the usage of `dgcov`:
>   When the server is compiled with `-DENABLE_GCOV=ON`, from object files are generated
>   `.gcno` and `.gcda` files.
>   `./mtr --gcov is_check_constraint` is invoking the following script calls:
>    - `./dgcov.pl --purge`, `./mtr is_check_constraint` and
>    - `./dgcov.pl --generate>/var/last_changes.dgcov`.
>   The `purge` flag is clearing `.gcda` files (and others extensions),
>   while running the test (which follows after the purge) new `.gcda` files are generated.
>   With dgcov `generate` flag, `gcov -i` (`intermediate format`) is called
>   on generated `<source-files-name>.gcda` files (`dbug.c.gcda` e.g.).
>   The patch is tested on `gcov 6.3` and `gcov 7.4` versions
>   and can be seen that resulting `.gcov` file for `6.3` creates
>   `<full path>.gcov` (`dbug.c.gcda.gcov` e.g.) file,
>   where `gcov 7.4+` is still creating `source-file-names.gcov`(`dbug.c.gcov`) files
>   as `gcov` in general is doing.
>   Results are stored in `./mysql-test/var/last_changes.dgcov`.
>   Note: for the newer versions of `gcov` intermediate format is
>   deprecated `-i` and should be used `--json-format`.

does it mean that eventually we'll have to rewrite dgcov to support
json format?

> 
> - This patch doesn't take gcov versions in account since is extracting
> filename of a `.gcov` file, which is generated with `gcov -i
> <source-file>.gcda` during `dgcov.pl --generate`.
> 
> - There is a check if file exists.
>   Reason for that is because some generated `gcov` files are outliers from above
>   rule (that a file generated in intermediate format(gcov) has the same source name).
>   Example 1: `aestables.cpp.gcda` is not generating `aestables.cpp.gcov`,
>   but only the files used in #include `modes.hpp.gcov  runtime.hpp.gcov`
>   Example 2: `my_sha256.cc.gcda` is generating `my_sha.ic.gcov`
> 
> - Gcov with out-of-source is not working, this patch will solve that
> (probably?)
>   Make sure to test `MTR_BINDIR` which gets set during out-of-source
>   build and generate an error when running `./mysql-test/mtr --gcov alias`.
> 
> diff --git a/mysql-test/dgcov.pl b/mysql-test/dgcov.pl
> index fbc5540e6973..f2bbdad4d20e 100755
> --- a/mysql-test/dgcov.pl
> +++ b/mysql-test/dgcov.pl
> @@ -161,8 +161,14 @@ sub gcov_one_file {
>      system($cmd)==0 or die "system($cmd): $? $!";
>    }

I don't see any changes since the last review. Is it the correct commit?

> -  # now, read the generated file
> -  open FH, '<', "$_.gcov" or die "open(<$_.gcov): $!";
> +  (my $filename = $_)=~ s/\.[^.]+$//; # remove extension

I think this contradicts your explanation in the commit comment.

You wrote that the file name can be either dbug.c.gcda.gcov or
dbug.c.gcov. Here $_ is dbug.c.gcda, you unconditionally remove .gcda
and then below you open $filename.gcov, that is dbug.c.gcov

Which means this won't work with an older gcov.
Is this your intention?

I think it's fine not to support old gcov. But this should be a
conscious decision, e.g. you can write in the commit comment "Now
dgcov.pl only supports gcov 7.4+" - if that's indeed what you want.

Also, if you do it this way, then there's no need to use s///, it's
simpler to do, like

  sub gcov_one_file {
    return unless /\.gcda$/;
+   my $filename= $`;
    unless ($opt_skip_gcov) {

or, may be even

  sub gcov_one_file {
    return unless /\.gcda$/;
+   my $gcov_file_path= "$File::Find::dir$`.gcov";
    unless ($opt_skip_gcov) {

> +  my $gcov_file_path= $File::Find::dir."/$filename.gcov";

still an extra slash

> +  if (! -f $gcov_file_path)
> +  {
> +    return;
> +  }

still not   return unless -f $gcov_file_path;

> +  open FH, '<', "$gcov_file_path" or die "open(<$gcov_file_path): $!";

still extra quotes

> +
>    my $fname;
>    while (<FH>) {
>      chomp;
> diff --git a/mysql-test/mysql-test-run.pl b/mysql-test/mysql-test-run.pl
> index 900ef736a455..2189529d82a7 100755
> --- a/mysql-test/mysql-test-run.pl
> +++ b/mysql-test/mysql-test-run.pl
> @@ -1791,7 +1791,7 @@ sub command_line_setup {
>    # --------------------------------------------------------------------------
>    # Gcov flag
>    # --------------------------------------------------------------------------
> -  if ( ($opt_gcov or $opt_gprof) and ! $source_dist )
> +  if ( ($opt_gcov or $opt_gprof) and (! $source_dist or -d $ENV{MTR_BINDIR}))
>    {
>      mtr_error("Coverage test needs the source - please use source dist");
>    }
> 
Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx