Hi Tom, Ulrich, Keith and community members,
Thank you very much for the feedback so far.
From my analysis, we have two choices either we can use NULL when getcwd () fails to return the path or pass an empty string “”.
If we pass NULL, absolute paths will work correctly from what I have seen in the code. Relative paths will be returned as is since they won’t work as getcwd () failed because someone deleted the directory or permission issues.
std::string
gdb_abspath (const char *path, const char *cwd)
{
gdb_assert (path != NULL && path[0] != '\0');
if (path[0] == '~')
return gdb_tilde_expand (path);
if (IS_ABSOLUTE_PATH (path) || cwd == NULL)
return path;
Since we passed NULL the above code will return as is.
If we pass “” an empty string, absolute paths will work correctly. Relative paths get that “/“ prepended and that is misleading since it gives the impression that the file is in root which is wrong especially if I am a non-root user.
return path_join (cwd, path);
In the above function the cwd size is one and a “/“ is appended.
I think it is better we keep it NULL and treat it as absolute, which is saying do not modify the path, but it won’t work.
>So even with this proposed patch, we'd still have a change in
>behavior (though no longer a crash) as compared to the version
>before Tom's commit above.
>Tom, any thoughts on how this should be handled? Should we
>add another check when passing captured_cwd to openp?
>Change captured_cwd to something like std::optional<std::string>?
As Ulrich suggested,
This is what I tried. If getcwd () fails, then we can set the captured_cwd to std::nullopt.
bash-5.3# git diff
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 7962acc784d..07668fdae0f 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -859,7 +859,9 @@ dwarf2_per_bfd::dwarf2_per_bfd (bfd *obfd, const dwarf2_debug_sections *names,
bool can_copy_)
: obfd (obfd),
can_copy (can_copy_),
- captured_cwd (current_directory == NULL ? "" : current_directory),
+ captured_cwd (current_directory != nullptr
+ ? std::optional<std::string> (current_directory)
+ : std::nullopt),
captured_debug_dir (debug_file_directory)
{
if (names == NULL)
@@ -6735,7 +6737,10 @@ try_open_dwop_file (dwarf2_per_bfd *per_bfd, const char *file_name, int is_dwp,
gdb::unique_xmalloc_ptr<char> absolute_name;
desc = openp (search_path, flags, file_name, O_RDONLY | O_BINARY,
- &absolute_name, per_bfd->captured_cwd.c_str ());
+ &absolute_name,
+ per_bfd->captured_cwd.has_value ()
+ ? per_bfd->captured_cwd->c_str ()
+ : nullptr);
if (desc < 0)
return NULL;
diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
index 15dd2abf3a1..308c34bf66a 100644
--- a/gdb/dwarf2/read.h
+++ b/gdb/dwarf2/read.h
@@ -757,8 +757,10 @@ struct dwarf2_per_bfd
abstract_to_concrete;
/* Current directory, captured at the moment that object this was
- created. */
- std::string captured_cwd;
+ created. If nullopt, the working directory was unavailable
+ which means getcwd failed either due to directory deletion or
+ permission issues. So paths should be treated as absolute. */
+ std::optional<std::string> captured_cwd;
/* Captured copy of debug_file_directory. */
std::string captured_debug_dir;
};
Let me know what you think. I have also written a small test for the same which I will share in v2 of this patch.
Thanks and regards,
Aditya.