[lldb-dev] Backslashes in command arguments

Mon Jan 12 17:03:03 PST 2015

The way Process does it is a little confusing to me.  It has this
ProcessProperties() class which takes a boolean, is_global.  If this value
is true, it appends some sub-properties to it, and if it's false, it just
initializes itself by calling itself again with true.  So false and true
make no difference, it's always the same instance (?)

Then, to further confuse matters, there's a single PropertyDefinition
array, where some of them are specified as global and some not via the
third argument.  When you call GetGlobalProperties(), it then just returns
the whole array.  There's not much choice it has, because it's all just one
array, so it can't really return a filtered array with only the items where
global==true.

I feel like the correct way to do this is to have two separate arrays.  One
for global properties and one for instance properties.  Then, instead of
inheriting from Properties, just have two instances of it in the class.  An
instance instance and a static instance.  This way you don't even need the
third argument to PropertyDefinition at all.  And the global properties are
added as a subcategory, such as interpreter.global.  This also makes it
clear to the user when they run "settings list" which settings are global
and which are not.  It might be clearer with an example, so I'll upload
this patch so you can see what I'm talking about.  If you don't like the
way I've done it let me know.

On Mon Jan 12 2015 at 3:01:21 PM Greg Clayton <gclayton at apple.com> wrote:

> Actually we do have command interpreter settings as Jim just pointed out
> to me, so feel free to add it there and hook the global settings up
> correctly for that with a static function on CommandInterpreter to get the
> escape character.
>
> Greg
>
> > On Jan 12, 2015, at 2:59 PM, Greg Clayton <gclayton at apple.com> wrote:
> >
> > Probably the command interpreter, but we have all the command
> interpreter settings on the debugger right now anyway, so don't change
> anything on that end..
> >
> >> On Jan 12, 2015, at 2:35 PM, Zachary Turner <zturner at google.com> wrote:
> >>
> >> Thanks for the response.  By the way, is this setting more appropriate
> on the CommandInterpreter instead of the debugger?
> >>
> >> On Mon Jan 12 2015 at 2:34:06 PM Greg Clayton <gclayton at apple.com>
> wrote:
> >>
> >>> On Jan 12, 2015, at 2:05 PM, Zachary Turner <zturner at google.com>
> wrote:
> >>>
> >>> There doesn't seem to be a good way to get access to the Debugger
> object from the Args class.  I tried changing Args to take a Debugger to
> its constructor, but it seems this isn't always possible, for example with
> anything having to do with OptionValue.
> >>> I could provide a default value of NULL for the Debugger, and if NULL
> it would fall back to the default escape character, but this seems awkward.
> >>>
> >>> Since I've declared this as a global property, why should I even need
> a Debugger instance?  Shouldn't the global settings be static on the
> Debugger so that they can be accessed without an instance?
> >>
> >>
> >> This is the problem. See how process does it:
> >>
> >> class Process {
> >>    static const ProcessPropertiesSP &
> >>    GetGlobalProperties();
> >> }
> >>
> >> The debugger should do the same thing. It should also add static
> accessors for all of the settings that are truly global settings. Looking
> at the global settings, they all seem to be set to be global values which
> probably isn't what we want. Why? Xcode creates a new Debugger for each
> debugging window that it creates and if someone types "setting set
> auto-confirm false" in one command interpreter, it shouldn't affect the
> other.
> >>
> >> So I would say we need to switch all debugger settings to be non-global
> (third initialized in each of the PropertyDefinition values should be set
> to "false" in g_properties in Debugger.cpp) and the only one that should
> remain global is your new escape character. Then Debugger should get a new
> static accessor:
> >>
> >> class Debugger
> >> {
> >>    static char
> >>    GetEscapeCharacter();
> >> };
> >>
> >> This in turn will access the global properties and return it to outside
> folks.
> >>
> >> Check out the ProcessProperties class in Process.h and check out its
> constructor. We will need to do something similar for the debugger's
> settings. Right now Debugger inherits directly from Properties, but we will
> need to change it to be like the process where there is a
> DebuggerProperties class. The global version gets constructed with a "bool
> is_global" set to false, and the instance ones get constructed with that
> set to true.
> >>
> >> The Process class is the model we will need to follow if you want to
> make this change and have a true global property that can be accessed
> without a Debugger instance.
> >>
> >>
> >>>
> >>> On Thu Jan 08 2015 at 3:00:30 PM <jingham at apple.com> wrote:
> >>> It's not just file names, you would also have to make sure there's
> nothing else that might be in command argument or option value that has a
> single & a double quote, since without the backslashes this would be
> impossible.
> >>>
> >>> If you really want to do this, I think it would be better to add a
> debugger setting for the "protection character".  Then this would be set to
> something else (maybe "#") on Windows, and backslash on all other systems.
> That way you could probably always find a protection character that you
> could use for whatever gnarly string you ended up having to pass through
> the command interpreter.
> >>>
> >>> Jim
> >>>
> >>>
> >>>> On Jan 8, 2015, at 2:55 PM, Zachary Turner <zturner at google.com>
> wrote:
> >>>>
> >>>> Actually ' is a valid filename character in Windows, but not ".  That
> being said, it's so rare, and having a backslash is so common that I would
> prefer the common case to be easy, and the rare case being either
> unsupported or difficult is ok with me.  So I'd still prefer to disable
> this backslash handling on Windows for now, and then fix ' on Windows
> separately in the future.
> >>>>
> >>>> On Thu Jan 08 2015 at 2:51:51 PM Zachary Turner <zturner at google.com>
> wrote:
> >>>> On Windows, that's not a valid file name, and in fact any file name
> that has an escaped character is not a valid filename.  I see what you're
> getting at though, that for non-Windows it's necessary.  Can I wrap the
> backslash handling in #ifndef _WIN32 then?
> >>>>
> >>>> On Thu Jan 08 2015 at 2:49:49 PM <jingham at apple.com> wrote:
> >>>> If I have a file called foo"bar'baz, how what would I put in the
> place of <AWKWARD NAME> for
> >>>>
> >>>> (lldb) file <AWKWARD NAME>
> >>>>
> >>>> Right now, I can do:
> >>>>
> >>>> (lldb) file foo\"bar\'baz
> >>>> Current executable set to 'foo"bar'baz' (x86_64).
> >>>>
> >>>> Jim
> >>>>
> >>>>
> >>>>> On Jan 8, 2015, at 2:31 PM, Zachary Turner <zturner at google.com>
> wrote:
> >>>>>
> >>>>> FWIW, Removing backslash handling from this function (essentially
> ignoring backslashes in command arguments) fixes about 12-15 tests on
> Windows with no other changes.  I see there's some logic in Args for
> encoding and decoding escape sequences, but this only happens if a
> particular command option is set, and that command only only appears to be
> set for the "prompt" command.  I'm not sure if removing the backslash logic
> from SetCommandString would interfere with this command in any way, but it
> doesn't seem like it would interfere with any other commands, unless I'm
> missing something.
> >>>>>
> >>>>> On Thu Jan 08 2015 at 1:43:16 PM Zachary Turner <zturner at google.com>
> wrote:
> >>>>> One thing causing many tests to fail on Windows is the presence of
> backslashes in argument.  Until now, I've worked around this in many cases
> by making sure that arguments with backslashes are always quoted.
> >>>>>
> >>>>> But there are some cases where this is not easy to guarantee and now
> I'm leaning towards (at least on Windows) allowing backslashes in argument
> strings.  The code in question comes from the function void
> SetCommandString (const char *command) in the file Args.cpp
> >>>>>
> >>>>> In particular, it implements special handling of whitespace, single
> quotes, double quotes, and backslashes.  For the case of backslashes it
> removes them from the string.
> >>>>>
> >>>>> What would be the implications of removing backslash handling from
> this function for all platforms?  I would prefer to keep platform specific
> code out of generic code, but I think this needs to be changed on Windows.
> >>>>> _______________________________________________
> >>>>> lldb-dev mailing list
> >>>>> lldb-dev at cs.uiuc.edu
> >>>>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
> >>>>
> >>>
> >>> _______________________________________________
> >>> lldb-dev mailing list
> >>> lldb-dev at cs.uiuc.edu
> >>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
> >>
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20150113/90a610ca/attachment.html>