diff mbox series

abipkgdiff: Fix comparing CTF kABIs using Oracle Linux RPMs

Message ID 20230217005743.2607886-1-guillermo.e.martinez@oracle.com
State New
Headers
Series abipkgdiff: Fix comparing CTF kABIs using Oracle Linux RPMs |

Commit Message

Guillermo E. Martinez Feb. 17, 2023, 12:57 a.m. UTC 
  Hello,

This patch is meant to fix how `abipkgdiff' using CTF origin is looking
for dependencies to analyze kABIs. IMHO `file_is_kernel_package'
function is relying on package name, so when `--ctf' is provided, it only
will work with executables under uncompressed directories and it never
will try to find `vmlinux.cfa' because it is just needed by KABIs. I
think that an option to force handle a packages as kernel would be nice
regardless the package name.

Please let me know your comments,
Thanks in advanced!,
guillermo
--

Using abipkgdiff to analyze kABIs from OL packages with CTF debug format
is not able to identify kernel packages, so the analysts is done as user
space RPMs, `file_is_kernel_package' function uses package name to
identify if the package contains kernel information, in OL distribution
the those packages are named as: `kernel-uek-core-*', then a wrong flow
is executed, additionally `vmlinux.ctfa' ship in a non debug package, so
the expected location is not satisfied .

	* src/abg-ctf-reader.cc (ctf::reader::find_ctfa_file): Use `find_file_under_dir'
	utility function to locate `vmlinux.ctfa' file.
	(ctf::reader::process_ctf_archive): Adjust dictionary name
	according to module name, removing characters after dot.
	* src/abg-tools-utils.cc (file_has_ctf_debug_info): Use `find_file_under_dir'
	utility function to locate `vmlinux.ctfa' file.
	(file_is_kernel_package): Adjust package name identifying a kernel package.
	(build_corpus_group_from_kernel_dist_under): Add `root' package directory
	to debug info array.

Signed-off-by: Guillermo E. Martinez <guillermo.e.martinez@oracle.com>
---
 src/abg-ctf-reader.cc  | 16 ++++++----------
 src/abg-tools-utils.cc | 14 ++++++++++++--
 2 files changed, 18 insertions(+), 12 deletions(-)

Comments

Guillermo E. Martinez Feb. 23, 2023, 4:04 p.m. UTC | #1 
Hello,

Any comment of this patch?

Thanks!,
guillermo

On Thu, Feb 16, 2023 at 06:57:43PM -0600, Guillermo E. Martinez wrote:
> Hello,
> 
> This patch is meant to fix how `abipkgdiff' using CTF origin is looking
> for dependencies to analyze kABIs. IMHO `file_is_kernel_package'
> function is relying on package name, so when `--ctf' is provided, it only
> will work with executables under uncompressed directories and it never
> will try to find `vmlinux.cfa' because it is just needed by KABIs. I
> think that an option to force handle a packages as kernel would be nice
> regardless the package name.
> 
> Please let me know your comments,
> Thanks in advanced!,
> guillermo
> --
> 
> Using abipkgdiff to analyze kABIs from OL packages with CTF debug format
> is not able to identify kernel packages, so the analysts is done as user
> space RPMs, `file_is_kernel_package' function uses package name to
> identify if the package contains kernel information, in OL distribution
> the those packages are named as: `kernel-uek-core-*', then a wrong flow
> is executed, additionally `vmlinux.ctfa' ship in a non debug package, so
> the expected location is not satisfied .
> 
> 	* src/abg-ctf-reader.cc (ctf::reader::find_ctfa_file): Use `find_file_under_dir'
> 	utility function to locate `vmlinux.ctfa' file.
> 	(ctf::reader::process_ctf_archive): Adjust dictionary name
> 	according to module name, removing characters after dot.
> 	* src/abg-tools-utils.cc (file_has_ctf_debug_info): Use `find_file_under_dir'
> 	utility function to locate `vmlinux.ctfa' file.
> 	(file_is_kernel_package): Adjust package name identifying a kernel package.
> 	(build_corpus_group_from_kernel_dist_under): Add `root' package directory
> 	to debug info array.
> 
> Signed-off-by: Guillermo E. Martinez <guillermo.e.martinez@oracle.com>
> ---
>  src/abg-ctf-reader.cc  | 16 ++++++----------
>  src/abg-tools-utils.cc | 14 ++++++++++++--
>  2 files changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/src/abg-ctf-reader.cc b/src/abg-ctf-reader.cc
> index 7159a578..ac0d3104 100644
> --- a/src/abg-ctf-reader.cc
> +++ b/src/abg-ctf-reader.cc
> @@ -336,12 +336,8 @@ public:
>      // for vmlinux.ctfa should be provided with --debug-info-dir
>      // option.
>      for (const auto& path : debug_info_root_paths())
> -      {
> -	ctfa_dirname = *path;
> -	ctfa_file = ctfa_dirname + "/vmlinux.ctfa";
> -	if (file_exists(ctfa_file))
> -	  return true;
> -      }
> +      if (tools_utils::find_file_under_dir(*path, "vmlinux.ctfa", ctfa_file))
> +        return true;
>  
>      return false;
>    }
> @@ -428,10 +424,10 @@ public:
>  	&& corpus_group())
>        {
>  	tools_utils::base_name(corpus_path(), dict_name);
> -
> -	if (dict_name != "vmlinux")
> -	  // remove .ko suffix
> -	  dict_name.erase(dict_name.length() - 3, 3);
> +	// remove .* suffix
> +	std::size_t pos = dict_name.find(".");
> +	if (pos != string::npos)
> +	  dict_name.erase(pos);
>  
>  	std::replace(dict_name.begin(), dict_name.end(), '-', '_');
>        }
> diff --git a/src/abg-tools-utils.cc b/src/abg-tools-utils.cc
> index 81f9aa75..d7f8b71f 100644
> --- a/src/abg-tools-utils.cc
> +++ b/src/abg-tools-utils.cc
> @@ -501,7 +501,7 @@ file_has_ctf_debug_info(const string& elf_file_path,
>  
>    // vmlinux.ctfa could be provided with --debug-info-dir
>    for (const auto& path : debug_info_root_paths)
> -    if (dir_contains_ctf_archive(*path, vmlinux))
> +    if (find_file_under_dir(*path, "vmlinux.ctfa", vmlinux))
>        return true;
>  
>    return false;
> @@ -1780,7 +1780,7 @@ file_is_kernel_package(const string& file_name, file_type file_type)
>      {
>        if (!get_rpm_name(file_name, package_name))
>  	return false;
> -      result = (package_name == "kernel");
> +      result = (string_begins_with(package_name, "kernel"));
>      }
>    else if (file_type == FILE_TYPE_DEB)
>      {
> @@ -2812,6 +2812,16 @@ build_corpus_group_from_kernel_dist_under(const string&	root,
>        vector<char**> di_roots;
>        di_roots.push_back(&di_root_ptr);
>  
> +#ifdef WITH_CTF
> +      shared_ptr<char> di_root_ctf;
> +      if (requested_fe_kind & corpus::CTF_ORIGIN)
> +        {
> +          di_root_ctf = make_path_absolute(root.c_str());
> +          char *di_root_ctf_ptr = di_root_ctf.get();
> +          di_roots.push_back(&di_root_ctf_ptr);
> +        }
> +#endif
> +
>        abigail::elf_based_reader_sptr reader =
>          create_best_elf_based_reader(vmlinux,
>                                       di_roots,
> -- 
> 2.39.1
>
Dodji Seketeli Feb. 27, 2023, 6:54 p.m. UTC | #2 
Hello Guillermo,

"Guillermo E. Martinez via Libabigail" <libabigail@sourceware.org> a
écrit:

> Hello,
>
> This patch is meant to fix how `abipkgdiff' using CTF origin is looking
> for dependencies to analyze kABIs. IMHO `file_is_kernel_package'
> function is relying on package name, so when `--ctf' is provided, it only
> will work with executables under uncompressed directories and it never
> will try to find `vmlinux.cfa' because it is just needed by KABIs. I
> think that an option to force handle a packages as kernel would be nice
> regardless the package name.
>
> Please let me know your comments,
> Thanks in advanced!,

Ahh, okay, I see.  Actually, I think the initial way in which
file_is_kernel_package detects that a package is a kernel package is
sub-optimal.  Rather than looking at the name of the package, I think it
should instead look for the 'vmlinuz' binary file inside the package.

I will soon post a patch that amends yours by fixing that initial issue
and adds the vmlinux.ctfa related fix to it.

[...]

Thanks for looking into this.

Cheers,
diff mbox series

Patch

diff --git a/src/abg-ctf-reader.cc b/src/abg-ctf-reader.cc
index 7159a578..ac0d3104 100644
--- a/src/abg-ctf-reader.cc
+++ b/src/abg-ctf-reader.cc
@@ -336,12 +336,8 @@  public:
     // for vmlinux.ctfa should be provided with --debug-info-dir
     // option.
     for (const auto& path : debug_info_root_paths())
-      {
-	ctfa_dirname = *path;
-	ctfa_file = ctfa_dirname + "/vmlinux.ctfa";
-	if (file_exists(ctfa_file))
-	  return true;
-      }
+      if (tools_utils::find_file_under_dir(*path, "vmlinux.ctfa", ctfa_file))
+        return true;
 
     return false;
   }
@@ -428,10 +424,10 @@  public:
 	&& corpus_group())
       {
 	tools_utils::base_name(corpus_path(), dict_name);
-
-	if (dict_name != "vmlinux")
-	  // remove .ko suffix
-	  dict_name.erase(dict_name.length() - 3, 3);
+	// remove .* suffix
+	std::size_t pos = dict_name.find(".");
+	if (pos != string::npos)
+	  dict_name.erase(pos);
 
 	std::replace(dict_name.begin(), dict_name.end(), '-', '_');
       }
diff --git a/src/abg-tools-utils.cc b/src/abg-tools-utils.cc
index 81f9aa75..d7f8b71f 100644
--- a/src/abg-tools-utils.cc
+++ b/src/abg-tools-utils.cc
@@ -501,7 +501,7 @@  file_has_ctf_debug_info(const string& elf_file_path,
 
   // vmlinux.ctfa could be provided with --debug-info-dir
   for (const auto& path : debug_info_root_paths)
-    if (dir_contains_ctf_archive(*path, vmlinux))
+    if (find_file_under_dir(*path, "vmlinux.ctfa", vmlinux))
       return true;
 
   return false;
@@ -1780,7 +1780,7 @@  file_is_kernel_package(const string& file_name, file_type file_type)
     {
       if (!get_rpm_name(file_name, package_name))
 	return false;
-      result = (package_name == "kernel");
+      result = (string_begins_with(package_name, "kernel"));
     }
   else if (file_type == FILE_TYPE_DEB)
     {
@@ -2812,6 +2812,16 @@  build_corpus_group_from_kernel_dist_under(const string&	root,
       vector<char**> di_roots;
       di_roots.push_back(&di_root_ptr);
 
+#ifdef WITH_CTF
+      shared_ptr<char> di_root_ctf;
+      if (requested_fe_kind & corpus::CTF_ORIGIN)
+        {
+          di_root_ctf = make_path_absolute(root.c_str());
+          char *di_root_ctf_ptr = di_root_ctf.get();
+          di_roots.push_back(&di_root_ctf_ptr);
+        }
+#endif
+
       abigail::elf_based_reader_sptr reader =
         create_best_elf_based_reader(vmlinux,
                                      di_roots,