Skip to content

Avoid losing information when using Debuginfo.from_location#2103

Merged
trefis merged 1 commit intoocaml:trunkfrom
Armael:debuginfo_to_loc
Oct 17, 2018
Merged

Avoid losing information when using Debuginfo.from_location#2103
trefis merged 1 commit intoocaml:trunkfrom
Armael:debuginfo_to_loc

Conversation

@Armael
Copy link
Copy Markdown
Member

@Armael Armael commented Oct 12, 2018 •
edited
Loading

It seems the middle end can emit semi-broken locations.
fun x -> Debuginfo.(to_location (from_location x)) is not the identity, because a Debuginfo.t contains strictly less information that a Location.t. In particular, it forgets about the position of start/end points in the input, and only remembers their position relative to the beginning of the line.

Currently this goes unnoticed because Debuginfo.to_location produces a mock location that is printed the same as the original one when calling Location.print_loc. However, this breaks #2096 in which I rely on the full information of the locations to look for the corresponding source code in the input.

This PR adds the missing information in Debuginfo.item so that to_location o from_location can be the identity (except for locations that would contain different filenames for start/end but I don't think this happens).

@Armael
Copy link
Copy Markdown
Member Author

Armael commented Oct 12, 2018

(cc @mshinwell )

@gasche
Copy link
Copy Markdown
Member

gasche commented Oct 13, 2018 •
edited
Loading

We looked at the code collectively this morning (me and Noémie, Basile, Axel, Maxime, Alexandre, Hamza).

Is it possible to write tests for this problem?

(You forgot to add a Changes entry)

There are two places in the code where the four current fields are used; should they be edited to also take the new fields into account?

  • Debuginfo.to_string, line 46 (I'm not sure we want to use the new fields here)
  • Debuginfo.compare, line 102 (it looks like a bug not to compare the new fields)

Otherwise the change looks nice and useful. Thanks!

@Armael
Copy link
Copy Markdown
Member Author

Armael commented Oct 13, 2018

Thanks to all of you for the thorough review!

Debuginfo.to_string, line 46 (I'm not sure we want to use the new fields here)

I don't think there's anything to do in this function indeed.

Debuginfo.compare, line 102 (it looks like a bug not to compare the new fields)

Good catch! Fixed.

Is it possible to write tests for this problem?

I added a few tests in compiler-libs/test_debuginfo.ml. However, they do not quite work at the moment: even though I'm supposedly loading compiler-libs, they do not find the Debuginfo module. I'd appreciate some advice; I pushed a separate commit (to be fixed) with the tests.

(You forgot to add a Changes entry)

I wasn't sure if it was needed for this kind of change. If you think it is, in which section should it belong?

@gasche
Copy link
Copy Markdown
Member

gasche commented Oct 13, 2018

Debuginfo is not part of compilerlibs/ocamlcommon but of compilerlibs/ocamlmiddleend. (I never remember so I call ./tools/ocamlobjinfo.opt on these files to check which units they include.) I have the impression that linking with ocamlmiddleend in the same way as ocamlcommon would require changing the ocamltest sources, so we could forget about the tests for now and leave that to a separate PR.

@gasche
Copy link
Copy Markdown
Member

gasche commented Oct 13, 2018

Re. Changes, I think you can go no-changelog on this one, but I wasn't sure so I assumed it was a mistake. (You could use the no-changes-entry-needed label to opt out, or say something about it.)
If we wanted a changes entry, I would go in the internals/compiler-libs section, but maybe consolidate it into a single entry with #2096 when that one gets rebased against the current PR.

@Armael
Copy link
Copy Markdown
Member Author

Armael commented Oct 13, 2018

OK. I removed the commit with the buggy tests.

@Armael
Copy link
Copy Markdown
Member Author

Armael commented Oct 16, 2018

@gasche ping

@Armael Armael mentioned this pull request Oct 16, 2018
Merged
@trefis
Copy link
Copy Markdown
Contributor

trefis commented Oct 17, 2018

Only the "changes" check fails on travis (I've just restarted it but it really doesn't care about your "no-change-entry-needed" label).
Given this, plus the fact that you've now approved this PR twice, as well as saying on #2096 that this PR had been merged, I'm going to go ahead and actually merge it.

@trefis trefis merged commit 6a8c46f into ocaml:trunk Oct 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gasche gasche gasche approved these changes

Assignees

No one assigned

Labels

no-change-entry-needed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Armael @gasche @trefis