Thanks for the review. > What fixes are needed on the macosx one? get_reg_num has a duplicate copy and offset value is incremented for registers which are part of another register. So in the end, we assign a wrong value to 'g-packet-size' key.
> Looks good. The only things I can see that we might want to change are: > > - should we rename "adjust-breakpoint-pc" to "breakpoint-pc-offset"? > - Should we make the adjust pc value signed? I don't know of any targets > that would need to adjust their PC values forward, but if this value is > signed, > we might want the value to be set to "-1" for linux? I renamed the key. It is also treated as signed now and -1 value is being used for x86_64 on Linux. Committed after making these changes. Regards, Abid > -----Original Message----- > From: Greg Clayton [mailto:[email protected]] > Sent: 17 October 2013 17:59 > To: Abid, Hafiz > Cc: [email protected] > Subject: Re: [lldb-dev] LLDB and gdbserver > > Comments inlined below. > > On Oct 17, 2013, at 6:11 AM, Abid, Hafiz <[email protected]> wrote: > > > My impression was that target definition file was just for describing the > target registers. But if we are adding connection specific information in it > then it makes sense to add this information there too. > > Yes, this is why I renamed it to "target-definition-file" instead of > "register- > definition-file". Any special target wide settings that are required for > debugging a target that we would normally use special LLDB packets for > (qRegisterInfo, qHostInfo, etc) can be put into this file to make it more > useful > and self sufficient (don't need a combo of "settings set" variables along with > the "target-definition-file" content). > > > The attached patch adds a new target definition file for Linux gdbserver. It > is quite similar to macosx file although there are a few differences. Some of > them are fixes that I will incorporate to macosx file too. > > What fixes are needed on the macosx one? > > > There are related changes in the ProcessGDBRemote.cpp. Tested with > gdbserver on Linux and it works fine. Ok to commit? > > Looks good. The only things I can see that we might want to change are: > > - should we rename "adjust-breakpoint-pc" to "breakpoint-pc-offset"? > - Should we make the adjust pc value signed? I don't know of any targets > that would need to adjust their PC values forward, but if this value is > signed, > we might want the value to be set to "-1" for linux? > > > > > Regards, > > Abid > > > >> -----Original Message----- > >> From: Greg Clayton [mailto:[email protected]] > >> Sent: 16 October 2013 19:21 > >> To: Abid, Hafiz > >> Cc: [email protected] > >> Subject: Re: [lldb-dev] LLDB and gdbserver > >> > >> The question is: do we need it? You could just add a key to the > >> target definition file. > >> > >> On Oct 16, 2013, at 10:10 AM, Abid, Hafiz <[email protected]> > wrote: > >> > >>> Greg, > >>> Thanks for doing python register stuff. It works great. > >>> > >>> I have also modified the patch to adjust the pc after hitting > >>> breakpoint. It > >> can now take target triple and pc adjust value as key value pairs. > >> Multiple pairs can be provided separated by space. > >>> setting set -- plugin.process.gdb-remote.adjust-breakpoint-pc > >>> x86_64-*-linux:1 > >>> > >>> Tested by connecting with gdbserver on Linux and it works fine. Ok > >>> to > >> commit? > >>> > >>> Regards, > >>> Abid > >>> > >>> > >>>> -----Original Message----- > >>>> From: Greg Clayton [mailto:[email protected]] > >>>> Sent: 15 October 2013 17:58 > >>>> To: Abid, Hafiz > >>>> Cc: [email protected] > >>>> Subject: Re: [lldb-dev] LLDB and gdbserver > >>>> > >>>> The XML patch is no longer needed. I checked in a similar way to do > >>>> this in python with revision 192646. The main reason is we already > >>>> had the ability to pass an python dictionary through the script > >>>> bridging API and parse the dictionary on the other side. The python > >>>> is easier to write, it can use code flow to construct the register > >>>> context programmatically, and it can use the LLDB defines and > >>>> enumeration values to avoid having to make up string values for > >>>> formats, encodings and everything else. We also already had code > >>>> that parsed a register definition from a dictionary that was used > >>>> in the > >> OperatingSystemPython plug-in. > >>>> > >>>> The details on the checkin are: > >>>> > >>>> Author: gclayton > >>>> Date: Mon Oct 14 19:14:28 2013 > >>>> New Revision: 192646 > >>>> > >>>> URL: http://llvm.org/viewvc/llvm-project?rev=192646&view=rev > >>>> Log: > >>>> <rdar://problem/14972424> > >>>> > >>>> When debugging with the GDB remote in LLDB, LLDB uses special > >>>> packets to discover the registers on the remote server. When those > >>>> packets aren't supported, LLDB doesn't know what the registers look > like. > >>>> This checkin implements a setting that can be used to specify a > >>>> python file that contains the registers definitions. The setting is: > >>>> > >>>> (lldb) settings set > >>>> plugin.process.gdb-remote.target-definition-file > >>>> /path/to/module.py > >>>> > >>>> Inside module there should be a function: > >>>> > >>>> def get_dynamic_setting(target, setting_name): > >>>> > >>>> This dynamic setting function is handed the "target" which is a > >>>> SBTarget, and the "setting_name", which is the name of the dynamic > >> setting to retrieve. > >>>> For the GDB remote target definition the setting name is > >>>> 'gdb-server-target- definition'. The return value is a dictionary > >>>> that follows the same format as the OperatingSystem plugins follow. > >>>> I have checked in an example file that implements the x86_64 GDB > >>>> register > >> set for people to see: > >>>> > >>>> examples/python/x86_64_target_definition.py > >>>> > >>>> This allows LLDB to debug to any archticture that is support and > >>>> allows users to define the registers contexts when the discovery > >>>> packets (qRegisterInfo, > >>>> qHostInfo) are not supported by the remote GDB server. > >>>> > >>>> A few benefits of doing this in Python: > >>>> 1 - The dynamic register context was already supported in the > >>>> OperatingSystem plug-in > >>>> 2 - Register contexts can use all of the LLDB enumerations and > >>>> definitions for things like lldb::Format, lldb::Encoding, generic > >>>> register numbers, invalid > >> registers > >>>> numbers, etc. > >>>> 3 - The code that generates the register context can use the > >>>> program to calculate the register context contents (like offsets, > >>>> register numbers, and > >>>> more) > >>>> 4 - True dynamic detection could be used where variables and types > >>>> could be read from the target program itself in order to determine > >>>> which registers are available since the target is passed into the > >>>> python function. > >>>> > >>>> This is designed to be used instead of XML since it is more dynamic > >>>> and code flow and functions can be used to make the dictionary. > >>>> > >>>> > >>>> Added: > >>>> lldb/trunk/examples/python/x86_64_target_definition.py > >>>> Modified: > >>>> lldb/trunk/include/lldb/Interpreter/ScriptInterpreter.h > >>>> lldb/trunk/include/lldb/Interpreter/ScriptInterpreterPython.h > >>>> lldb/trunk/scripts/Python/python-wrapper.swig > >>>> lldb/trunk/source/Interpreter/ScriptInterpreterPython.cpp > >>>> lldb/trunk/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp > >>>> lldb/trunk/source/Plugins/Process/gdb- > >>>> remote/GDBRemoteCommunicationClient.cpp > >>>> lldb/trunk/source/Plugins/Process/gdb- > >>>> remote/GDBRemoteRegisterContext.cpp > >>>> lldb/trunk/source/Plugins/Process/gdb- > >>>> remote/GDBRemoteRegisterContext.h > >>>> lldb/trunk/source/Plugins/Process/gdb- > remote/ProcessGDBRemote.cpp > >>>> lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h > >>>> > >>>> > >>>> On Oct 11, 2013, at 6:52 AM, Abid, Hafiz <[email protected]> > >> wrote: > >>>> > >>>>> Ping. There are 2 patches in this series waiting review. > >>>>> > >>>>> Thanks, > >>>>> Abid > >>>>> > >>>>>> -----Original Message----- > >>>>>> From: [email protected] > >>>>>> [mailto:[email protected]] > >>>>>> On Behalf Of Abid, Hafiz > >>>>>> Sent: 09 October 2013 17:33 > >>>>>> To: [email protected] > >>>>>> Subject: Re: [lldb-dev] LLDB and gdbserver > >>>>>> > >>>>>> Hi All, > >>>>>> The attached patch implements reading register description of the > >>>>>> remote target from the xml file. The path of the file is provided > >>>>>> through a settings as a first step. We can make lldb search > >>>>>> though xml files and find correct one based on arch and g packet > >>>>>> size on top of this patch. After this patch and my previous patch > >>>>>> (regarding adjusting pc when breakpoint is hit) go in, we can > >>>>>> connect to gdbserver like shown below. I am also attaching an xml > >>>>>> file that I used for > >>>> testing on Ubuntu 12.04 on x86-64. > >>>>>> > >>>>>> I was not aware what will be the best way to check for the > >>>>>> presence of libXML. I am using an already present macro > 'CLANG_HAVE_LIBXML'. > >>>>>> > >>>>>> How does it all look? > >>>>>> > >>>>>> Regards, > >>>>>> Abid > >>>>>> > >>>>>> lldb ~/demos/act > >>>>>> Current executable set to '/home/abidh/demos/act' (x86_64). > >>>>>> (lldb) settings set -- > >>>>>> plugin.process.gdb-remote.register-definition-file > >>>>>> /home/abidh/work/llvm/src/tools/lldb/xml/x86_64_gdbserver.xml > >>>>>> (lldb) setting set -- > >>>>>> plugin.process.gdb-remote.adjust-breakpoint-pc > >>>>>> 1 > >>>>>> (lldb) gdb-remote 10000 > >>>>>> > >>>>>> > >>>>>>> -----Original Message----- > >>>>>>> From: [email protected] > >>>>>>> [mailto:[email protected]] > >>>>>>> On Behalf Of Abid, Hafiz > >>>>>>> Sent: 04 October 2013 17:45 > >>>>>>> To: Greg Clayton > >>>>>>> Cc: [email protected] > >>>>>>> Subject: Re: [lldb-dev] LLDB and gdbserver > >>>>>>> > >>>>>>> Hi Greg, > >>>>>>> I am attaching a patch that implements the 2nd part as discussed > >>>>>>> below > >>>>>> i.e. > >>>>>>> to adjust the pc after hitting breakpoint. The response of > >>>>>>> qHostInfo packet is checked for ' adjusts_breakpoint_pc' key. If > >>>>>>> found, it gives the value by which the pc should be adjusted. In > >>>>>>> its absence, a new setting ' plugin.process.gdb- remote .adjust- > >> breakpoint-pc' > >>>>>>> can be used to provide this information. The default of this > >>>>>>> settings is > >> 0. > >>>>>>> If a user is connecting to a stub that needs the debugger to > >>>>>>> adjust the pc (e.g. gdbserver) then user can adjust the value of > >>>>>>> this settings > >>>>>> accordingly. > >>>>>>> > >>>>>>> I tested the new setting and it is working fine. As I don't have > >>>>>>> a stub that sends qHostInfo with this new key, so that code is > >>>>>>> un-tested. OK > >>>>>> to commit? > >>>>>>> > >>>>>>> Regards, > >>>>>>> Abid > >>>>>>> > >>>>>>>> -----Original Message----- > >>>>>>>> From: Greg Clayton [mailto:[email protected]] > >>>>>>>> Sent: 05 September 2013 19:26 > >>>>>>>> To: Abid, Hafiz > >>>>>>>> Cc: [email protected] > >>>>>>>> Subject: Re: [lldb-dev] LLDB and gdbserver > >>>>>>>> > >>>>>>>> > >>>>>>>> On Sep 5, 2013, at 9:16 AM, Abid, Hafiz > <[email protected]> > >>>>>> wrote: > >>>>>>>> > >>>>>>>>> I was able to make the LLDB work with gdbserver. Running, > >>>>>>>>> stopping, > >>>>>>>> source stepping were working ok. I needed to patch 2 areas to > >>>>>>>> make it > >>>>>>> work. > >>>>>>>> I would like comments on those changes before I can get them > >>>>>>>> ready for submission. > >>>>>>>>> > >>>>>>>>> If dynamic register info is not available then > >>>>>>>>> GDBRemoteRegisterContext is > >>>>>>>> relying on hardcoded registers for ARM. I have to added similar > >>>>>>>> hard-coded registers for x86_64. Would it make any sense if we > >>>>>>>> keep GDBRemoteRegisterContext for reading/writing the register > >>>>>>>> packets > >>>> only. > >>>>>>>> The task of translating those packets should be left to some > >>>>>>>> higher level classes. Perhaps something like > >>>>>>>> GDBRemoteRegisterContext_arm, > >>>>>>>> GDBRemoteRegisterContext_x86_64 etc. Or for the time being, > >>>>>>>> hardcoding is considered ok. > >>>>>>>> > >>>>>>>> The only hard coded registers should be for ARM as this was > >>>>>>>> needed for legacy iOS support. Any new registers should use a > >>>>>>>> new plugin setting that supplies an XML file that describes the > >>>>>>>> registers. The setting should be something like: > >>>>>>>> > >>>>>>>> (lldb) settings set > >>>>>>>> plugin.process.gdb-remote.register-definition-file > >>>>>>>> /path/to/registers.xml > >>>>>>>> > >>>>>>>> The XML register description should supply the same information > >>>>>>>> as the qRegisterInfo packet for all registers. Then the > >>>>>>>> GDBRemoteDynamicRegisterInfo class will need to be able to set > >>>>>>>> itself up using an XML file. Another way to do this would be to > >>>>>>>> supply a python > >>>>>>> file. > >>>>>>>> We have Python bridging objects available where you could > >>>>>>>> easily make a new python callback where a python dictionary > >>>>>>>> could be > >>>>>> returned. > >>>>>>>> Python might also be more useful because you could create > >>>>>>>> classes that know the common register numbers for certain > >>>>>>>> architectures and > >>>>>> ABIs... > >>>>>>>> > >>>>>>>> So the flow would be: > >>>>>>>> - debugging starts with debugserver > >>>>>>>> - we stop for the first time and "qRegisterInfo0" packet is > >>>>>>>> sent, and the unsupported response of "$#00" is returned. > >>>>>>>> - we grab the value of the > >>>>>>>> "plugin.process.gdb-remote.register-definition- > >>>>>>>> file" setting, and if it is set, we parse that file > >>>>>>>> - else fallback to hard coded registers. > >>>>>>>> > >>>>>>>> So I would avoid adding anymore hard coded registers to LLDB > >>>>>>>> otherwise we will end up with a mess in LLDB with all the > >>>>>>>> different gdb servers that we can attach to. > >>>>>>>> > >>>>>>>>> It seems that debugserver decrements the pc after stopping on > >>>>>>> breakpoint. > >>>>>>>> To find the stop reason, code in > >>>>>>>> ProcessGDBRemote::SetThreadStopInfo() > >>>>>>>> checks for breakpoint on pc. But gdbserver does not decrement > >>>>>>>> the pc in this case. I had to duplicate the above check for (pc > >>>>>>>> - 1) and then decrement the pc accordingly. I was wondering if > >>>>>>>> there is some good way to distinguish if I am connected to > >>>>>>>> debugserver or gdbserver. Can I make use of some of the new > >>>>>>>> packets added by LLDB or perhaps add some option in the gdb- > remote command? > >>>>>>>> > >>>>>>>> I would modify the qHostInfo to return a new key/value pair like: > >>>>>>>> "adjusts_breakpoint_pc:1;" or "adjusts_breakpoint_pc:0;" so we > >>>>>>>> know for certain architectures if we need to manually adjust the > PC. > >>>>>>>> Then we use a LazyBool instance variable in > >>>>>>>> GDBRemoteCommunicationClient to detect this setting via the > >>>>>>>> qHostInfo packet. If the "adjusts_breakpoint_pc" key/value > >>>>>>>> isn't specified in the qHostInfo packet, or if the qHostInfo > >>>>>>>> packet isn't supported, we should fall back to a > >>>>>>> setting: > >>>>>>>> > >>>>>>>> (lldb) settings set > >>>>>>>> plugin.process.gdb-remote.adjust-breakpoint-pc > >>>>>>>> true > >>>>>>>> (lldb) settings set > >>>>>>>> plugin.process.gdb-remote.adjust-breakpoint-pc > >>>>>>>> false > >>>>>>>> > >>>>>>>> > >>>>>>>> So with all settings when using GDB server, we try to detect > >>>>>>>> things dynamically (registers and other settings like the > >>>>>>>> adjust breakpoint PC), and if that fails, we fall back to manual > settings. > >>>>>>>> > >>>>>>>> Greg > >>>>> > >>>>> _______________________________________________ > >>>>> lldb-dev mailing list > >>>>> [email protected] > >>>>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev > >>> > >>> <pc_adjust.patch> > > > > <pc_adjust.patch> _______________________________________________ lldb-dev mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
