Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Save more file information from initial call... #56

Merged
merged 6 commits into from
Dec 27, 2024

Conversation

rocky
Copy link
Collaborator

@rocky rocky commented Dec 15, 2024

In particular, resolve the initial source directory and store that as _Dbg_init_dir. Resolve the name of main bash program and add that to canonicalized filenames.

@jansorg I think this fixes the second problem you mentioned in #50 Please double check though.

@rocky rocky force-pushed the correct-initial-cwd-and-canonicalize-main-file branch from 4a499ba to f41e58a Compare December 15, 2024 20:24
In particular, resolve the initial source directory and store that as
_Dbg_init_dir. Resolve the name of main bash program and add that to
canonicalized filenames.
@rocky rocky force-pushed the correct-initial-cwd-and-canonicalize-main-file branch from f41e58a to 3838e65 Compare December 15, 2024 20:34
@jansorg
Copy link
Collaborator

jansorg commented Dec 17, 2024

@rocky Thank you, this is fixing the problem!
Unfortunately, it seems to add a regression that break with a relative path doesn't work anymore, e.g. break ./sources/lib.bash:2.
break with an absolute path is working well.

I've attached a screenshot of my session.

image

lib/hook.sh Outdated Show resolved Hide resolved
bashdb.in Outdated Show resolved Hide resolved
bashdb.in Outdated Show resolved Hide resolved
rocky and others added 3 commits December 17, 2024 05:50
Co-authored-by: Joachim Ansorg <[email protected]>
Co-authored-by: Joachim Ansorg <[email protected]>
Co-authored-by: Joachim Ansorg <[email protected]>
@rocky
Copy link
Collaborator Author

rocky commented Dec 17, 2024

@rocky Thank you, this is fixing the problem! Unfortunately, it seems to add a regression that break with a relative path doesn't work anymore, e.g. break ./sources/lib.bash:2. break with an absolute path is working well.

I've attached a screenshot of my session.

image

Thanks for checking and the changes. I will try to look at and address this problem over the weekend.

@rocky
Copy link
Collaborator Author

rocky commented Dec 23, 2024

@rocky Thank you, this is fixing the problem! Unfortunately, it seems to add a regression that break with a relative path doesn't work anymore, e.g. break ./sources/lib.bash:2. break with an absolute path is working well.
I've attached a screenshot of my session.
image

Thanks for checking and the changes. I will try to look at and address this problem over the weekend.

@jansorg When I went to look at this over the weekend the image was missing. Would you upload it again or give a sequence of instructions?

When I debug I see that in relative path resolution favor the initial current working directory over the current working directory, and that can sometimes be wrong. But it is hard to know if this is exactly the problem. Nevermind. I see that this is only an image problem in my reply, not what you posted originally.

The key issue is that the "break" command is not finding the file when using a relative path. As you report, using an absolute path works fine.

In some cases though automatic file loading no longer happens.
In particular, on "list" and "break" commands.
@rocky
Copy link
Collaborator Author

rocky commented Dec 23, 2024

@jansorg with 896a12f the example you give now works. However, this also disables the automatic file loading of files with relative file names not previously encountered.

Instead, you must specify an explicit "load" here. The automatic loads of relative file names happened in the "list" and "break" commands. With this change, you will get an "internal error" message instead.

Addressing that will take more work and effort. I am not sure though it is that important. What do you think?

@jansorg
Copy link
Collaborator

jansorg commented Dec 23, 2024

I'll be able to test your changes end of this week. Thank you for looking into this!

No automatic loading of relative paths seems okay to me, unless I misunderstood what how it works.
As far as I understand, instead of a "break ./a/b.sh" an additional "load ./a/b.sh" is needed first. I didn't even know that this was a thing and my IDE integration is always executing a "load" command first in such a case.

lib/fns.sh Outdated
@@ -197,6 +197,9 @@ function _Dbg_linespec_setup {
typeset -ri is_function=${word[1]}
line_number=${word[0]}
full_filename=$(_Dbg_is_file "$filename")
if [[ -z "$full_filename" ]] ; then
full_filename=$(_Dbg_resolve_expand_filename "$filename")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is quoting needed here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested with quoting and it doesn't seem to hurt. Personally, I find the quotes within quotes confusing, and more generally the multiple substitution passes weird, but this is bash after all.

So I've now made the change.

@rocky
Copy link
Collaborator Author

rocky commented Dec 23, 2024

I'll be able to test your changes end of this week. Thank you for looking into this!

No automatic loading of relative paths seems okay to me, unless I misunderstood what how it works. As far as I understand, instead of a "break ./a/b.sh" an additional "load ./a/b.sh" is needed first. I didn't even know that this was a thing and my IDE integration always executes a "load" command first in such a case.

In stepping, the debugger may encounter relative paths in BASH_SOURCE. It turns these into absolute paths and saves the mapping as soon as possible, to avoid the problem of the current working directory changing inside the debugged program. So in that respect, the auto conversion always happens, and has to happen.

In addition, a user can issue debugger commands like "list" or "break" fabricating any name the user wants. It is here that I guess we hitting places where we are not converting the filename to its full name or storing canonical information about the file.

@jansorg
Copy link
Collaborator

jansorg commented Dec 27, 2024

I've tested your branch and as far as I can tell, it's working great! Thank you for fixing this problem!

@jansorg jansorg merged commit 251cefe into bash-5.2 Dec 27, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants