[Lldb-commits] [PATCH] Add process launch --enable-aslr option, tweak handling of flag

[Todd Fiala]

Great, thanks!


On Mon, Aug 18, 2014 at 3:22 PM, <jingham at apple.com> wrote:

>
> > On Aug 18, 2014, at 3:18 PM, Todd Fiala <tfiala at google.com> wrote:
> >
> > On Mon, Aug 18, 2014 at 2:47 PM,  <jingham at apple.com> wrote:
> >
> > > On Aug 18, 2014, at 12:45 PM, Todd Fiala <tfiala at google.com> wrote:
> > >
> > > On Mon, Aug 18, 2014 at 11:22 AM,  <jingham at apple.com> wrote:
> > > I don't think we have any other instances where we use two flags to
> express "do x" and "don't do x".
> > > For the long options is isn't such a big deal but we try not to use up
> more short options than necessary, since this gets to be a crowded space.
> > >
> > > I'd be happy to use long-option only here.
> > >
> > >   Maybe "process launch --enable-aslr <true/false>", which mirrors the
> setting anyway?
> > >
> > >
> > > Sounds good.  But the fallback behavior when process launch doesn't
> specify anything is currently to disable ASLR if the target.disable-aslr
> setting is true.  Are you interested in reversing that setting to
> enable-aslr?  If not, then maybe we go with just extending 'process launch
> --disable-aslr <true/false>'.
> > >
> > > In sum, I'm for either
> > >
> > > (1) changing to either `process launch --enable-aslr <true/false>` and
> change the `settings target.disable-aslr' to match the name as the fallback
> setting, or
> > >
> > > (2) keeping the target.disable-aslr setting, and extending `process
> launch --disable-aslr` to take true/false.  (But - note - this does get
> into what Chandler called out before as being somewhat long in the tooth -
> `process launch --disable-aslr false` when you want ASLR.)
> >
> > I like option 2.  You generally aren't turning on aslr, that's something
> the system does or does not have on.  You're disabling the system's aslr
> for this launch.
> >
> > Yeah, I started having pangs of a potential misleading option that
> requires more explaining when I wrote this comment in the patch:
> >
> > // Determine whether we will disable ASLR or leave it in the default
> state (i.e. enabled if the platform supports it).
> >
> > So I think disable is right.
> >
> > I am logically behind that argument.  I talked to Ed Maste earlier today
> as I had done some research on this over the weekend on FreeBSD and found
> that it was still in patch state (i.e. not part of FreeBSD proper).  So
> there it definitely would be always disabled, because the default state is
> "not there".  Enable would certainly be misleading there.  Chandler made
> the argument for emitting a warning/error in that case (i.e. you're asking
> for something that cannot be).
> >
> > Probably the conceptual dislike here is we have a disable mechanism but
> the disable is the default.  So shutting off the disable is saying disable
> = no.  Just a bit wordy.  But accurate.
> >
> >
> > I like option 2.  You generally aren't turning on aslr, that's something
> the system does or does not have on.  You're disabling the system's aslr
> for this launch.
> >
> > Let's just leave it at that.  I'll adjust.  So, to make sure I
> understand, are we keeping the -A for that as the short option?  (I think
> yes, and we're just getting rid of the --enable-aslr since this will be
> --disable-aslr false).
> >
>
> Yes, that seems right to me.
>
> Jim
>
>
>
> > -Todd
> >
> > As for the length of the command, either this isn't going to be
> something you type often (and even if you are you'll type:
> >
> > pr la --d 0
> >
> > ) or if you use it a lot, you'll make an alias for it.
> >
> >
> > >
> > > Thoughts on that?  I'll code up whatever we decide on.
> > >
> > > If backward  compat for option (1) is a concern, we could continue to
> accept `settings set disable-aslr <true/false>` and just have it do the
> right thing.
> > >
> > > -Todd
> > >
> > > Jim
> > >
> > > > On Aug 17, 2014, at 10:48 PM, Todd Fiala <todd.fiala at gmail.com>
> wrote:
> > > >
> > > > This change modifies the logic used to set the
> eLaunchFlagDisableASLR ProcessLaunchInfo setting for inferior process
> launching.  Now, if 'process launch' is provided with either --disable-aslr
> or --enable-aslr, then the launch flag is set accordingly.  If niether
> --disable-aslr or --enable-aslr are specified, then the setting for
> target.disable-aslr is used to determine the setting or clearing of the
> eLaunchFlagDisableASLR setting.  The target.disable-aslr setting currently
> defaults to true, so the default behavior when nothing is specified on the
> 'process launch' (i.e. 'run' command) is to disable ASLR.
> > > >
> > > > --
> > > > -Todd
> > > >
> <tfiala_enable-aslr.diff>_______________________________________________
> > > > lldb-commits mailing list
> > > > lldb-commits at cs.uiuc.edu
> > > > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
> > >
> > > _______________________________________________
> > > lldb-commits mailing list
> > > lldb-commits at cs.uiuc.edu
> > > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
> > >
> > >
> > >
> > > --
> > > Todd Fiala |   Software Engineer |     tfiala at google.com |
> 650-943-3180
> >
> >
> >
> >
> > --
> > Todd Fiala |   Software Engineer |     tfiala at google.com |
> 650-943-3180
>
>


-- 
Todd Fiala | Software Engineer | tfiala at google.com | 650-943-3180
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20140818/e89c5bb0/attachment.html>