Page MenuHomePhabricator

[source maps] Ensure all valid source maps are added instead of failing with the first invalid one
ClosedPublic

Authored by wallace on Mar 31 2020, 5:13 PM.

Details

Summary

Several lldb-vscode users have noticed that when a source map rule is invalid (because a folder doesn't exist anymore), the rest of the source maps from their configurations are not applied.
This happens because lldb-vscode executes a single "settings set target.source-map" command with all the source maps and LLDB processes them one by one until one fails.

Instead of doing this, we can process in LLDB all the source map rules and apply the valid ones instead of failing fast.

Diff Detail

Event Timeline

wallace created this revision.Mar 31 2020, 5:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2020, 5:13 PM
clayborg accepted this revision.Mar 31 2020, 7:09 PM

This will do what the user intends more of the time. Good catch

This revision is now accepted and ready to land.Mar 31 2020, 7:09 PM
labath added a comment.Apr 1 2020, 2:08 AM

It seems somewhat odd for a command to return error (one of the effects of that for instance is to abort processing of batch scripts), but still perform some changes. It might be more appropriate to call those warnings. However, reporting warnings from here would require some plumbing, and the new behavior is definitely more reasonable than the old one, so I don't think this patch should be blocked on that.

lldb/source/Interpreter/OptionValuePathMappings.cpp
73

does this actually change anything?

113–114

If there are multiple non-existing paths, this will just return the last one right? We should list all of them.

146

does this change anything?

lldb/test/API/functionalities/source-map/TestTargetSourceMap.py
45

It would be good to follow this up with a settings show, both to test/document the expected final value of the setting, and also to catch any issues early.

wallace marked 4 inline comments as done.Apr 1 2020, 10:41 AM
wallace added inline comments.
lldb/source/Interpreter/OptionValuePathMappings.cpp
73

Indeed. It's used in line 70 as the index to replace in the replace operation

113–114

good catch

146

Same as above, it's used in line 144

lldb/test/API/functionalities/source-map/TestTargetSourceMap.py
45

good one

wallace updated this revision to Diff 254268.Apr 1 2020, 12:09 PM

address comments

This revision was automatically updated to reflect the committed changes.
MaskRay added a subscriber: MaskRay.Apr 1 2020, 1:14 PM

address comments

@wallace labath made comments after the differential had been approved. So it looks like he may have further questions. It was probably better waiting for an explicit ack instead of being in a haste.

Thanks for the heads up. I think I got a false impression of how comments after accept vs requesting changes work in this repo.

labath added a comment.Apr 2 2020, 1:29 AM

Thanks for the heads up. I think I got a false impression of how comments after accept vs requesting changes work in this repo.

The rules on that are somewhat fuzzy, but there has been a recent effort to formalize that. Generally, in a situation like this (where a patch already got one LGTM and so is "green"), I would have used "request changes" if I had major objections and was sure I want to see the patch again. If I only had cosmetic comments/trivial comments, I would have clicked "approve" and trusted you to apply them before committing. Not doing either of those is a sort of a "meh" position, where I'm uncomfortable lgtm-ing something, but I also don't want to reject it (as that's fairly aggressive). In this case, I don't think you have done anything really wrong strictly speaking, but it would have been polite to wait a while so that I have a chance to respond.

lldb/source/Interpreter/OptionValuePathMappings.cpp
73

Yes, but that would have also worked (I think) if you had left the increment in the for loop. My question was why did you need to move the increment into the loop body.