Debugger inverts BUILD_PATH_PREFIX_MAP before loading paths stored in cm? by pirbo · Pull Request #14055 · ocaml/ocaml

pirbo · 2025-05-26T12:03:39Z

The whole purpose of BUILD_PATH_PREFIX_MAP is to be able to rewrite prefixes of absolute paths written into compilation unit. ocamldebug reads these paths. But, when it is launched in the folder where the sources have been compiled, theses prefixes don't make sense. This is why ocamldebug currently fails to find any information (sources/printers/...) about programs compiled by dune.

This patch makes the hypothesis that when it launched while BUILD_PATH_PREFIX_MAP is set, it means that it is launched from the repository in which sources have been built using this value for the environment variable. Therefore, it "inverts" it to find files on disk.

This restores the usability of ocamldebug in "the setting it used to work" before dune 3.0.

gasche · 2025-05-26T15:50:32Z

We could decide to use this as a heuristic rather than a hard rule: look at the inverted path first, but look at the provided path if that fails. This would make the new system trivially compatible with what the current compiler today, but also more complex. I'm willing to make the bet that this complexity is not necessary -- that BUILD_PATH_PREFIX_MAP should always be inverted by the debugger, never ignored -- but others might disagree, and this is an option we have for later if compatibility turns out to be an issue.

gasche

I made a code review, assuming that we are okay with the change of behavior as previously mentioned. This looks fine overall, with nitpick comments.

gasche · 2025-05-26T16:28:56Z

utils/build_path_prefix_map.ml

+  | Some { target; source } ->
+    let is_prefix =
+      String.length target <= String.length path
+        && String.equal target (String.sub path 0 (String.length target)) in


This is now in the stdlib! String.starts_with ~prefix:target path.

(I see that this is reused from code above. Maybe you could factor out some has_prefix ~prefix str and replace_prefix ~prefix ~new_prefix str auxiliary helper functions?)

So, I used String.starts_with but I didn't refactored more because make_target is dead code that I'm willing to erased if it is still unused once we are happy with the behaviors around BUILD_PATH_PREFIX_MAP so I don't want to touch it (as long as it is not used).

gasche · 2025-05-26T16:30:29Z

debugger/symbols.ml

+                           else s)
+            s
+            (Misc.revert_build_path_prefix_map abs_e))
+        !dirs (input_value ic)


This is getting a bit complex and I think it could go to an auxiliary function, if you don't mind.

Indeed, done (and actually I took that opportunity to give to the auxiliary function a better semantic (in my opinion))

gasche · 2025-05-26T16:39:25Z

utils/build_path_prefix_map.mli

    it just returns [path]. *)
+
+val revert_all : map -> path -> path list
+(** [revert_all map path] finds all targets in [map]


Should we use the word "invert" (as in your PR description) or "revert" (as in the code)? I'm fine with either options. I find that "invert" (so here I guess invert_all?) makes some sense in relation to the mathematical notion (inverse map, inverse of a relation, etc.).

Yes, "invert" everywhere it is...

dbuenzli · 2025-05-26T21:48:25Z

utils/misc.mli

+val revert_build_path_prefix_map: string -> string list
+(** Returns the potential pathes from which the given path can
+    originate from before rewrite using [BUILD_PATH_PREFIX_MAP]
+    environment variable. *)


pathes -> paths. Also it would be nice to indicate the order of the result in this doc string.

pirbo · 2025-05-27T10:03:45Z

We could decide to use this as a heuristic rather than a hard rule

I'm not sure it "adds anything" but it certainly doesn't hurt and the added complexity is OK (once the machinery put in an auxiliary function) therefore... Done!

pirbo · 2025-08-25T07:36:50Z

I don't understand what is the next action to be done here and who has to do it. And, as it has been stalled for 3 months now. I fear it's on me, is it?

gasche

The ping is mostly what was required. Can we do a quick recap of where this PR stands, though? If I understand correctly, it started as a PR that would change the debugger behavior, but in the end you removed that part of the change and the PR only adds the ability to invert prefix-map paths. Right? So presumably other behavior changes are coming later?

I'm fine with the current state, and/but I think it would be nice to have an accurate summary of the PR before we merge.

gasche · 2025-08-25T08:36:18Z

utils/misc.mli

    variable. *)

+val invert_build_path_prefix_map: string -> string list
+(** Returns the potential paths (in decreasing order of priority) from


I would use "in priority order" as in build_path_prefix_map.mli, the notion of increasing/decreasing priority tends to be confusing and subject to opposite conventions.

pirbo · 2025-08-25T15:20:05Z

Attempt to clarify what this PR does:
1/ (stowaway but mandatory cleanup) #12139 has been reverted but we forgot to remove its entry in the CHANGES. I'm doing it here. It has nothing to do with the rest of "this PR".
2/ This PR does change the debugger behavior but backward incompatibilities would be super super super specific cases:

Before, the debugger was simply ignoring BUILD_PATH_PREFIX_MAP. Therefore, it was including as-it to its loadpath paths that the compiler had rewrite using BUILD_PATH_PREFIX_MAP.
Now, when adding to its loadpath paths written by the compiler in cmo files as potential locations of source files, the debugger detects the one that are in the target of BUILD_PATH_PREFIX_MAP rewrites and, for these, if inverting the rewrite gives an existing folder, it adds to its loadpath this "path with rewrite inverted" else it still adds the path as written in the cmo file.

(so breakages occur when 1/BUILD_PATH_PREFIX_MAP is set while launching ocamldebug which by itself is an exotic situation for now and 2/ there are paths for which the rewrite in the backward direction gives an existing folder (aka the sources are still there) but really you meant to add to the loadpath the rewrited path.)

This PR will not have any direct follow-up in the repository. It's potential follow-up would be in dune repository where I'll try to argue that as dune calls the compilers with BUILD_PATH_PREFIX_MAP="$PWD/_build/default/=/dune_workspace/", it should offer a command to call ocamldebug with the same definition for BUILD_PATH_PREFIX_MAP so that we can more easily debug what we've just compiled...

gasche · 2025-08-25T18:52:05Z

Thanks for the summary! Merging.

(There is a transient testsuite failure in weak_array_par, same output as there.

gasche approved these changes May 26, 2025

View reviewed changes

dbuenzli reviewed May 26, 2025

View reviewed changes

pirbo force-pushed the debugger_revert_bppm branch from 4ec815b to a62923c Compare May 27, 2025 10:00

nojb assigned gasche May 28, 2025

pirbo force-pushed the debugger_revert_bppm branch from 877f0e8 to 0e80bb3 Compare August 25, 2025 07:32

gasche reviewed Aug 25, 2025

View reviewed changes

pirbo added 4 commits August 25, 2025 16:26

Misc.invert_build_path_prefix_map

6b21d6a

Apply BUILD_PATH_PREFIX_MAP to pathes found in files by the debugger

427e1a6

Change reverted for now

ed4d3bb

Changes

ab592af

pirbo force-pushed the debugger_revert_bppm branch from 0e80bb3 to ab592af Compare August 25, 2025 14:27

gasche merged commit 2c69170 into ocaml:trunk Aug 25, 2025
20 of 24 checks passed

pirbo deleted the debugger_revert_bppm branch August 25, 2025 19:34

gasche mentioned this pull request Dec 19, 2025

Support for BUILD_PATH_PREFIX_MAP ocaml/dune#7540

Closed

Conversation

pirbo commented May 26, 2025

Uh oh!

gasche commented May 26, 2025

Uh oh!

gasche left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pirbo commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pirbo commented Aug 25, 2025

Uh oh!

gasche left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pirbo commented Aug 25, 2025

Uh oh!

gasche commented Aug 25, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pirbo commented May 27, 2025 •

edited

Loading