Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 310304

Summary: [launch][cdi] GDB (DSF) Hardware Debugging Launcher ignores application program path
Product: [Tools] CDT Reporter: John Dallaway <john>
Component: cdt-debug-dsf-gdbAssignee: John Cortell <john.cortell>
Status: RESOLVED FIXED QA Contact: Marc Khouzam <marc.khouzam>
Severity: normal    
Priority: P3 CC: ajin, elaskavaia.cdt, john.cortell, pawel.1.piech
Version: 6.0   
Target Milestone: 7.0   
Hardware: PC   
OS: Linux   
Whiteboard:
Attachments:
Description Flags
Proposed changes to Startup page
john.cortell: iplog-
Prototype support for the Startup changes I proposed (CDI only)
none
Complete fix
john.cortell: iplog-
Complete fix (updated)
john.cortell: iplog-
cdi-gdb.log
none
dsf-gdb.log none

Description John Dallaway CLA 2010-04-23 12:22:05 EDT
Build Identifier: Eclipse I20100420-0800, CDT HEAD

This is a CDI-DSF parity bug.

The DSF-GDB hardware debugging launcher ignores the C/C++ application program path specified in the launch configuration. The CDI-GDB hardware debugging launcher passes this information to GDB correctly.

The other DSF-GDB launchers use org.eclipse.cdt.dsf.gdb.launching.FinalLaunchSequence which includes a launch Step for fGDBBackend.getProgramPath() + fCommandFactory.createMIFileExecAndSymbols().

The DSF-GDB hardware debugging launcher uses org.eclipse.cdt.debug.gdbjtag.core.GDBJtagDSFFinalLaunchSequence which does not include an equivalent launch Step.

Note that it is important for GDB to have access to the symbol table before it connects to a remote target. The fCommandFactory.createMIFileExecAndSymbols() Step should therefore occur early in the launch sequence.

Reproducible: Always

Steps to Reproduce:
1. Create a DSF-GDB hardware launch configuration and specify an application file.
2. Select "Use remote target" in the launch configuration and specify "load" in the initialization commands text box.
2. Attempt to launch.
3. Observe launch fails because the executable file not known to GDB:

416,874 &"load\n"
416,874 &"No executable file specified.\n"
416,875 &"Use the \"file\" or \"exec-file\" command.\n"
416,875 8^error,msg="No executable file specified.\nUse the \"file\" or \"exec-file\" command."
416,885 (gdb)
Comment 1 John Cortell CLA 2010-04-23 12:50:17 EDT
Interesting. It appears the CDI JTAG launcher also does not MIFileExecAndSymbols, and apparently no one has complained(?). How has gdb gotten by without knowing the symbolics information in these debug sessions?

I have a patch ready for the DSF side, but I need this little mystery resolved first...
Comment 2 John Cortell CLA 2010-04-23 12:51:40 EDT
Nevermind. Just re-read your description, and the answer is there :-)
Comment 3 John Cortell CLA 2010-04-23 13:03:52 EDT
I think there's a misunderstanding here. The JTAG launcher, by design apparently, doesn't implicitly tell gdb to process the executable. There's a Startup tab. In in you can specify the executable and a symbols file. By default, the executable is the program specified in the Main tab, but the option to process the file at all is disabled. Also, the IGDBJtagDevice implementor must provide the gdb commands to load the image.
Comment 4 John Dallaway CLA 2010-04-26 05:39:03 EDT
(In reply to comment #1)
> Interesting. It appears the CDI JTAG launcher also does not
> MIFileExecAndSymbols, and apparently no one has complained(?). How has gdb
> gotten by without knowing the symbolics information in these debug sessions?

The CDI JTAG launcher passes the application executable file path to GDB as a command line parameter when the GDB process is started so it is not necessary to use MIFileExecAndSymbols before symbol lookup can be performed. Ref:

  AbstractGDBCDIDebugger#createSession()

(In reply to comment #3)
> I think there's a misunderstanding here. The JTAG launcher, by design
> apparently, doesn't implicitly tell gdb to process the executable. There's a
> Startup tab. In in you can specify the executable and a symbols file. By
> default, the executable is the program specified in the Main tab, but the
> option to process the file at all is disabled. Also, the IGDBJtagDevice
> implementor must provide the gdb commands to load the image.

I have now investigated the behaviour of the "Load image" and "Load symbols" launch options. The behaviour is, as you mention, customisable. But most users will not have the necessary knowledge to create their own IGDBJtagDevice implementor. In the CDI JTAG launcher, the executable file (including symbol table) is available by default in the way many (most?) users would expect. In the DSF JTAG launcher the executable file and symbol table file (usually the same file) are not available by default and none of the provided IGDBJtagDevice implementors provide the standard GDB commands to make use of these files ("file", "exec-file", "symbol-file"). The default implementations of the "Load image" and "Load symbols" launch options provide the more specialized "restore" and "add-symbol-file" GDB commands respectively.

I think there are two usability issues here:

a) Regular GDB command behaviour (MIFileExecAndSymbols) is not provided within the DSF JTAG launcher by default. This is a source of confusion for users familiar with the older CDI JTAG launcher. Personally, I assumed that the two launchers would have identical behaviour since the two user interfaces are identical.

b) For many users, it will be necessary to use a "file" command in the "initialization commands" text box with the DSF JTAG launcher and to copy the executable file path from the "Main" tab into the "Startup" tab. Users will question why this is necessary and what is the purpose of the "C/C++ Application" field on the "Main" tab.

I think the easiest way to address these usability issues would be to add an optional "MIFileExecAndSymbols" Step early in the DSF JTAG launch sequence to provide equivalent behaviour to the CDI JTAG launcher. Enabled by default.
Comment 5 John Cortell CLA 2010-04-27 11:32:25 EDT
(In reply to comment #4)
One thing I absolutely agree with you on is that the behavior should be the same for CDI and DSF. However, I don't think the right direction here is to make DSF behave like CDI, simply because I don't think the CDI behavior makes a whole lot of sense. I think the DSF behavior makes more sense, but suffers from the usability issues you mentioned.

The reason I think the CDI behavior is flawed is that the launch configuration is clearly meant to give the user fine-grained control over what executable is loaded into memory and where the symbolics information is taken from. To expose that level of control in the GUI and then just load the project's output artifact automatically when launching gdb doesn't make much sense to me. So, in that respect, I think the DSF behavior is spot on. However, I think we need to improve the default behavior so that things 'just work' out of the box for most users. That would mean automatically populating the "Symbols file name" in the same way that "Image file name" is populated, and having the "Load image" and "Load symbols" checkboxes enabled by default. Perhaps, too, adding 'exec-file' to the default handling of IGDBJtagDevice.doLoadImage().

And, of course, I would adjust the CDI behavior to be the same.

Would this meet your expectations?

> (In reply to comment #1)
> > Interesting. It appears the CDI JTAG launcher also does not
> > MIFileExecAndSymbols, and apparently no one has complained(?). How has gdb
> > gotten by without knowing the symbolics information in these debug sessions?
> 
> The CDI JTAG launcher passes the application executable file path to GDB as a
> command line parameter when the GDB process is started so it is not necessary
> to use MIFileExecAndSymbols before symbol lookup can be performed. Ref:
> 
>   AbstractGDBCDIDebugger#createSession()
> 
> (In reply to comment #3)
> > I think there's a misunderstanding here. The JTAG launcher, by design
> > apparently, doesn't implicitly tell gdb to process the executable. There's a
> > Startup tab. In in you can specify the executable and a symbols file. By
> > default, the executable is the program specified in the Main tab, but the
> > option to process the file at all is disabled. Also, the IGDBJtagDevice
> > implementor must provide the gdb commands to load the image.
> 
> I have now investigated the behaviour of the "Load image" and "Load symbols"
> launch options. The behaviour is, as you mention, customisable. But most users
> will not have the necessary knowledge to create their own IGDBJtagDevice
> implementor. In the CDI JTAG launcher, the executable file (including symbol
> table) is available by default in the way many (most?) users would expect. In
> the DSF JTAG launcher the executable file and symbol table file (usually the
> same file) are not available by default and none of the provided IGDBJtagDevice
> implementors provide the standard GDB commands to make use of these files
> ("file", "exec-file", "symbol-file"). The default implementations of the "Load
> image" and "Load symbols" launch options provide the more specialized "restore"
> and "add-symbol-file" GDB commands respectively.
> 
> I think there are two usability issues here:
> 
> a) Regular GDB command behaviour (MIFileExecAndSymbols) is not provided within
> the DSF JTAG launcher by default. This is a source of confusion for users
> familiar with the older CDI JTAG launcher. Personally, I assumed that the two
> launchers would have identical behaviour since the two user interfaces are
> identical.
> 
> b) For many users, it will be necessary to use a "file" command in the
> "initialization commands" text box with the DSF JTAG launcher and to copy the
> executable file path from the "Main" tab into the "Startup" tab. Users will
> question why this is necessary and what is the purpose of the "C/C++
> Application" field on the "Main" tab.
> 
> I think the easiest way to address these usability issues would be to add an
> optional "MIFileExecAndSymbols" Step early in the DSF JTAG launch sequence to
> provide equivalent behaviour to the CDI JTAG launcher. Enabled by default.
Comment 6 John Dallaway CLA 2010-04-27 13:35:31 EDT
(In reply to comment #5)
> (In reply to comment #4)
> One thing I absolutely agree with you on is that the behavior should be the
> same for CDI and DSF. However, I don't think the right direction here is to
> make DSF behave like CDI, simply because I don't think the CDI behavior makes
> a whole lot of sense. I think the DSF behavior makes more sense, but suffers
> from the usability issues you mentioned.
> 
> The reason I think the CDI behavior is flawed is that the launch configuration
> is clearly meant to give the user fine-grained control over what executable is
> loaded into memory and where the symbolics information is taken from. To
> expose that level of control in the GUI and then just load the project's
> output artifact automatically when launching gdb doesn't make much sense to
> me.

The default behaviour of the CDI JTAG launcher's "Load image" checkbox is to restore an image from file using a "restore" GDB command (reverse of "dump"). The default behaviour of the CDI JTAG launcher's "Load symbols" checkbox is to _add_ symbols using an "add-symbol-file" GDB command. Symbols already loaded from the application image are not discarded. According to the GDB documentation, "add-symbol-file" is typically used with code that has been dynamically loaded. The nature of these commands makes me think that the original author may have intended these options to supplement the default behaviour of the launcher in specific situations only. Perhaps we should ask Doug Schaefer what he intended here?

> So, in that respect, I think the DSF behavior is spot on. However, I
> think we need to improve the default behavior so that things 'just work' out
> of the box for most users. That would mean automatically populating the
> "Symbols file name" in the same way that "Image file name" is populated, and
> having the "Load image" and "Load symbols" checkboxes enabled by default.
> Perhaps, too, adding 'exec-file' to the default handling of
> IGDBJtagDevice.doLoadImage().

For the default behaviour to "just work" for most people, "exec-file" and "symbol-file" behaviours are essential. Can you envisage a situation where the user would need to disable these behaviours?

Personally, I think the "Load image" and "Load symbols" checkboxes tend to obscure the precise behaviour of the launcher. I would choose to add GDB commands to the initialisation commands text box rather than use a checkbox to enable a behaviour which is not well specified. But requiring a "file <filepath>" command seems counter-intuitive as discussed in comment 4.

> And, of course, I would adjust the CDI behavior to be the same.
> 
> Would this meet your expectations?

I don't have specific expectations, but I'm not sure that changing the behaviour of the CDI JTAG launcher for existing users will be well received.
Comment 7 Marc Khouzam CLA 2010-04-27 15:03:49 EDT
I'm adding Andy to this bug.  He may have some opinions about the intended behavior.  He is the one that implemented this launch for DSF-GDB.
Comment 8 Marc Khouzam CLA 2010-04-27 15:05:07 EDT
Also added Elena who might have some opinion on any changes to the JTAG launch.
Comment 9 John Cortell CLA 2010-04-27 15:17:51 EDT
I will yield here to Doug or anyone else who might see it your way. Ultimately,
my interpretation of the feature doesn't match yours. The design of the JTAG
launch configuration tells *me* the choice of what program to load on the
target and what symbols file to load in the debugger should be explicitly
stated in the Startup page, and I'm all for preloading those to improve the
experience for the user. You believe we should have gdb process the project's
output regardless of the Startup page. I understand the CDI plugin behaves that
way and that works out well for you. That doesn't make it right in my opinion.
I think we can accomplish the same end result but keep to what I believe the
intended design of the feature was, by doing the things I pointed out in
comment # 5. Yes, it would change the behavior of CDI but, unless I'm mistaken,
it would give you the same behavior you're seeing now. It seems to me your
adamant on getting the CDI behavior in DSF, exactly as it is carried out in
CDI. I'm more interested in getting you the same end result as in CDI but doing
it in a way that matches how I think the feature was designed to work. But we
can't come to an agreement, so I'm more than happy to let someone else take the
reigns on this one.
Comment 10 Elena Laskavaia CLA 2010-04-27 15:39:10 EDT
The history behind it - I think originally launch configuration were very
upset if binary is not specified on the main tab - what is why it there. 
Load image is added because sometimes image it not the same as binary - in this
case binary is just ignored. Symbols field is there because it is needed - symbols are usually in separate files. Did it clarify anything?
Comment 11 John Dallaway CLA 2010-04-28 03:54:35 EDT
(In reply to comment #10)
> The history behind it - I think originally launch configuration were very
> upset if binary is not specified on the main tab - what is why it there. 
> Load image is added because sometimes image it not the same as binary - in this
> case binary is just ignored. Symbols field is there because it is needed -
> symbols are usually in separate files. Did it clarify anything?

Alena, thank you for these insights.

If I have understood you correctly, the design intention was that users can use the "Load symbols" checkbox to override the default behaviour of the CDI JTAG launcher. So perhaps the naming of this checkbox is a source of confusion. "Load alternative symbols" or "Load additional symbols" would be more accurate. Many users of the CDI JTAG debugger do not enable this option since the debug symbols within the executable file are sufficient.

The default behaviour of the CDI JTAG debugger is not to load any image, so the naming of the "Load image" checkbox is reasonable.
Comment 12 John Dallaway CLA 2010-04-28 06:19:14 EDT
(In reply to comment #9)
> I will yield here to Doug or anyone else who might see it your way.
> Ultimately, my interpretation of the feature doesn't match yours. The design
> of the JTAG launch configuration tells *me* the choice of what program to
> load on the target and what symbols file to load in the debugger should be
> explicitly stated in the Startup page, and I'm all for preloading those to
> improve the experience for the user.

> You believe we should have gdb process the project's output regardless of the
> Startup page. I understand the CDI plugin behaves that way and that works out
> well for you. That doesn't make it right in my opinion.  I think we can
> accomplish the same end result but keep to what I believe the intended design
> of the feature was, by doing the things I pointed out in comment # 5. Yes, it
> would change the behavior of CDI but, unless I'm mistaken, it would give you
> the same behavior you're seeing now. It seems to me your adamant on getting
> the CDI behavior in DSF, exactly as it is carried out in CDI. I'm more
> interested in getting you the same end result as in CDI but doing it in a way
> that matches how I think the feature was designed to work. But we can't come
> to an agreement, so I'm more than happy to let someone else take the
> reigns on this one.

John, yes, we see things differently. Both perspectives are valid.

I raised the bug report as a result of user confusion in the behaviour of the DSF JTAG debugger. We agree that something should be to be done to improve usability. Whatever changes are made, it seems that the important CDI-DSF parity requirements are:

a) Ability to include "file" (or "exec-file" and "exec-symbol") in the launch sequence without needing to cut and paste the application executable filepath.

b) Loading of symbols *before* connecting to the target in the launch sequence so that GDB can lookup and present the symbolic name corresponding to the initial value of the program counter.

With these requirements satisfied, users can further customise the launch using the "Initialization commands" text box without difficulty.

If you decide to adopt the proposal you present in comment 5, I would suggest the following additional changes to achieve CDI-DSF parity:

a) Move the doLoadSymbol() Step to precede the doRemote() Step in the launch sequence.

b) Add a new ("Basic" ?) IGDBJtagDevice implementor (or modify the "Generic" implementor) to implement:

     doLoadImage():   "exec-file" then "load"
     doLoadSymbol():  "symbol-file"
Comment 13 John Cortell CLA 2010-04-28 10:01:26 EDT
(In reply to comment #12)
I've been giving this some more thought, and frankly, I'm not happy with my own proposal. It is also flawed in that the user ends up choosing an executable on the main tab but then we effectively ignore it. And the idea of prepopulating the image and symbol fields is problematic. So, I've come up with an alternative. The idea is to allow the user to specify in the Startup page that he wants to use whatever executable he chose in the Main tab. See attached screenshot. In this modified GUI, the defaults for the Startup page would be 
- the "Load image" checkbox is unchecked
- its associated "Use project application" radio button is selected
- the "Load symbols" checkbox is checked
- its associated "Use project application" radio button is selected

This should maintain the current behavior in CDI, at least conceptually.
Comment 14 John Cortell CLA 2010-04-28 10:02:16 EDT
Created attachment 166318 [details]
Proposed changes to Startup page
Comment 15 Elena Laskavaia CLA 2010-04-28 10:52:11 EDT
It seems reasonable. Only I don't like word application.
Should be "Use project's binary"? I know it is called application in Main tab
but it is still sounds weird. It should have a tooltip too.
(image: Use project application defined in the Main tab).
(symbols: Load symbols from project application defined in the Main tab)
Comment 16 John Cortell CLA 2010-04-28 11:01:56 EDT
(In reply to comment #15)
> It seems reasonable. Only I don't like word application.
> Should be "Use project's binary"? I know it is called application in Main tab
> but it is still sounds weird. It should have a tooltip too.
> (image: Use project application defined in the Main tab).
> (symbols: Load symbols from project application defined in the Main tab)

Yes. It's weird but, as you figured, I was going for consistency. I'll go with "Use project binary" and add a tooltip to clarify.
Comment 17 John Cortell CLA 2010-05-04 19:31:46 EDT
Created attachment 167065 [details]
Prototype support for the Startup changes I proposed (CDI only)

This is first draft at the GUI changes I talked about and the underlying support in the CDI debugger. John, if you could try it and see if it provides the behavior you desire. You'll have to create a new launch configuration. Remember to switch the launcher to CDI.

Basically, this 
- no longer specifies the project binary when launching gdb
- by default, has the Load Image and Load Symbolics checked
- by default, uses the project binary
- changes to DefaultGDBJtagDeviceImpl to hopefully provide desired/expected behavior

Once we have agreement on the CDI behavior, I'll update the DSF one to work the same way.
Comment 18 John Dallaway CLA 2010-05-05 05:49:41 EDT
(In reply to comment #17)
> Created an attachment (id=167065) [details]
> Prototype support for the Startup changes I proposed (CDI only)
> 
> This is first draft at the GUI changes I talked about and the underlying
> support in the CDI debugger. John, if you could try it and see if it provides
> the behavior you desire. You'll have to create a new launch configuration.
> Remember to switch the launcher to CDI.
> 
> Basically, this 
> - no longer specifies the project binary when launching gdb
> - by default, has the Load Image and Load Symbolics checked
> - by default, uses the project binary
> - changes to DefaultGDBJtagDeviceImpl to hopefully provide desired/expected
> behavior
> 
> Once we have agreement on the CDI behavior, I'll update the DSF one to work the
> same way.

John, I can now perform a basic launch using the behaviours enabled by the "Load Image" checkbox. Unlike the GDB "load" command, the "restore" command does not set the program counter to the entry point of the loaded code, so I have to specify this myself using the "Set program counter" option in the dialog box. I may contribute an alternative IGDBJtagDevice implementor which uses "load" instead of "restore" at some point.

A few minor issues:

The "symbol-file" command is executed twice for the CDI JTAG launcher. You need to add a call to commands.clear() before the call to gdbJtagDevice.doRemote().

There's a GDB/MI error message early in the launch sequence:

  546-gdb-set new-console on
  546^error,msg="No symbol table is loaded.  Use the \"file\" command."
  547 symbol-file /home/jld/test/myfile
  &"symbol-file /home/jld/test/myfile\n"
  547^done

It looks like the ordering of these two commands should be swapped in the launch sequence.

There's a missing colon character at the end of one of the instances of the "Use project binary:" dialog box label.

Otherwise, your patch is working for CDI JTAG launches.
Comment 19 John Cortell CLA 2010-05-05 08:04:10 EDT
(In reply to comment #18)
> John, I can now perform a basic launch using the behaviours enabled by the
> "Load Image" checkbox. Unlike the GDB "load" command, the "restore" command
> does not set the program counter to the entry point of the loaded code, so I
> have to specify this myself using the "Set program counter" option in the
> dialog box. I may contribute an alternative IGDBJtagDevice implementor which
> uses "load" instead of "restore" at some point.

Interesting. OK. Well, I see no reason why we shouldn't use 'load' instead or 'restore' in the generic implementation. The user still has the ability to set a different PC via the GUI, and as long as we run that step after the load, we should be OK. Do you agree?

> The "symbol-file" command is executed twice for the CDI JTAG launcher. You need
> to add a call to commands.clear() before the call to gdbJtagDevice.doRemote().
> 
> There's a GDB/MI error message early in the launch sequence:
> 
>   546-gdb-set new-console on
>   546^error,msg="No symbol table is loaded.  Use the \"file\" command."
>   547 symbol-file /home/jld/test/myfile
>   &"symbol-file /home/jld/test/myfile\n"
>   547^done
> 
> It looks like the ordering of these two commands should be swapped in the
> launch sequence.

Huh? The setting the new-console property requires a symbol table to be loaded? That doesn't make much sense to me.I'll check with the gdb guys. Thing is, I don't remember seeing that failure (I'll have to check again). My launches obviously don't connect to anything, but they go through the motions and I didn't see this error. What gdb are you using? Can you post the entire gdb log for the launch?

> There's a missing colon character at the end of one of the instances of the
> "Use project binary:" dialog box label.

Will fix

> Otherwise, your patch is working for CDI JTAG launches.

Thanks. I'll fix the above issues and work on the DSF part and post up a new patch sometime today.
Comment 20 John Dallaway CLA 2010-05-05 08:52:39 EDT
(In reply to comment #19)
> Interesting. OK. Well, I see no reason why we shouldn't use 'load' instead or
> 'restore' in the generic implementation. The user still has the ability to set
> a different PC via the GUI, and as long as we run that step after the load, we
> should be OK. Do you agree?

I have no problem with using "load" for the generic implementation. It seems more generic than "restore" to me. But some existing users may be depending on "restore" functionality. You could use "load" if the image offset field is empty, otherwise use "restore", but that would make the interpretation of the launch settings even more opaque.

> > There's a GDB/MI error message early in the launch sequence:
> > 
> >   546-gdb-set new-console on
> >   546^error,msg="No symbol table is loaded.  Use the \"file\" command."
> >   547 symbol-file /home/jld/test/myfile
> >   &"symbol-file /home/jld/test/myfile\n"
> >   547^done
> > 
> > It looks like the ordering of these two commands should be swapped in the
> > launch sequence.
> 
> Huh? The setting the new-console property requires a symbol table to be
> loaded? That doesn't make much sense to me. I'll check with the gdb guys.

I've discovered that "set new-console" is a Cygwin-specific GDB command. I'm using a Linux host, so "new-console" has no special meaning. Bug 146725 seems relevant.
Comment 21 John Cortell CLA 2010-05-05 09:08:16 EDT
(In reply to comment #20)
> (In reply to comment #19)
> > Interesting. OK. Well, I see no reason why we shouldn't use 'load' instead or
> > 'restore' in the generic implementation. The user still has the ability to set
> > a different PC via the GUI, and as long as we run that step after the load, we
> > should be OK. Do you agree?
> 
> I have no problem with using "load" for the generic implementation. It seems
> more generic than "restore" to me. But some existing users may be depending on
> "restore" functionality. You could use "load" if the image offset field is
> empty, otherwise use "restore", but that would make the interpretation of the
> launch settings even more opaque.

Prior to my changes, did you have to explicitly set the PC?
Comment 22 John Dallaway CLA 2010-05-05 09:32:56 EDT
(In reply to comment #21)
> Prior to my changes, did you have to explicitly set the PC?

No, because I was not using the "Load image" behaviour, I was specifying "load" in the "Run commands" text box instead.
Comment 23 John Cortell CLA 2010-05-05 09:55:50 EDT
(In reply to comment #22)
> No, because I was not using the "Load image" behaviour, I was specifying "load"
> in the "Run commands" text box instead.

Hm. I don't see any easy or clean path here other than to change the generic behavior, and if there really are people out there who need the PC unchanged after launch, then this is not an option. The proper way to do this would involve GUI enhancements but more importantly API changes, and it's technically too late for the latter. I guess the options for the user for now will be to specify the PC explicitly or provide a IGDBJtagDevice implementation.
Comment 24 Elena Laskavaia CLA 2010-05-05 10:45:26 EDT
I am voting for using load in generic if address is not specified and
restore if it is. Finding out $pc is not very obvious and it will break
lot of launch configs who rely on it.
Comment 25 John Cortell CLA 2010-05-05 10:55:37 EDT
(In reply to comment #24)
> I am voting for using load in generic if address is not specified and
> restore if it is. Finding out $pc is not very obvious and it will break
> lot of launch configs who rely on it.

Fair enough. I'll make it so.
Comment 26 John Cortell CLA 2010-05-05 19:55:51 EDT
Created attachment 167248 [details]
Complete fix

John, please verify with CDI and DSF, if you can. Again, I don't have an environment to test this in so my testing is based on looking at the gdb trace. Some real-world testing would be much appreciated.

This patch includes extracting some methods from AbstractCLaunchDelegate into CDebugUtils so that they can be more widely shared. I found quite a bit of code duplication surrounding using the program and application settings in a CDT launch configuration. This fix would have added more, but I decided against that. After Helios, I'll be looking at getting DSF-GDB and EDC to make use of these methods and deprecate theirs. I'll open a separate bug for that.
Comment 27 John Dallaway CLA 2010-05-06 02:50:23 EDT
(In reply to comment #26)
> Created an attachment (id=167248) [details]
> Complete fix
> 
> John, please verify with CDI and DSF, if you can. Again, I don't have an
> environment to test this in so my testing is based on looking at the gdb trace.
> Some real-world testing would be much appreciated.

John, no problem. I should be able to take a look tomorrow.
Comment 28 John Cortell CLA 2010-05-06 16:16:33 EDT
Created attachment 167385 [details]
Complete fix (updated)

The changes for 305943 touched a number of files that my patch touched, and in the same areas--creating a less than pleasurable merging task. Here's an updated patch. I'm committing this to HEAD to avoid having to go through this again. I'm reasonably confident in the fix.
Comment 29 John Dallaway CLA 2010-05-07 08:10:18 EDT
(In reply to comment #28)
> Created an attachment (id=167385) [details]
> Complete fix (updated)
> 
> The changes for 305943 touched a number of files that my patch touched, and in
> the same areas--creating a less than pleasurable merging task. Here's an
> updated patch. I'm committing this to HEAD to avoid having to go through this
> again. I'm reasonably confident in the fix.

Testing from the current HEAD.

The CDI JTAG debugger is working well.

When using the DSF JTAG debugger with "Load Image" checked and the "Image Offset" field empty, I see the following message from "exec-file" in the GDB/MI log:

805,136 (gdb) 
805,137 &"load /home/jld/path/to/myfile\n"
805,137 ~"Loading section .text, size 0xb25a lma 0x108000\n"
805,138 +download,{section=".text",section-size="45658",total-size="762051"}

...

806,225 ~"Start address 0x108000, load size 48437\n"
806,236 ~"Transfer rate: 42 KB/sec, 320 bytes/write.\n"
806,237 10^done
806,240 (gdb) 
806,240 &"exec-file /home/jld/path/to/myfile\n"
806,240 ~"A program is being debugged already.\n"
806,240 ~"Are you sure you want to change the file? (y or n) [answered Y; input not from terminal]\n\
"
806,241 ^done
806,241 (gdb) 
806,241 &"\n"
806,241 ~"Current language:  auto; currently asm\n"
806,241 ^done
806,241 (gdb) 

I have verified that you can eliminate this message by specifying the "exec-file" command before the "load" command in the doLoadImage() method. In fact, the "exec-file" command may be unnecessary when passing the file name to the "load" command directly.

Otherwise, the DSF JTAG launch proceeds correctly. The "symbol-file" behaviour when the "Symbol offset" field is empty is working well.
Comment 30 John Cortell CLA 2010-05-07 09:29:57 EDT
(In reply to comment #29)
John, can you send me the gdb log from both the CDI and DSF sessions. The ordering of the load and exec-file commands are the same in both, since that code is in the common, default jtagdevice implementation. So, the fact that the order is fine for CDI but not DSF is an indication that there is some unintended difference between CDI and DSF elsewhere. The logs should help me figure what that difference is.
Comment 31 John Dallaway CLA 2010-05-07 10:18:25 EDT
Created attachment 167476 [details]
cdi-gdb.log

CDI GDB log - no prompt when executing "exec-file"
Comment 32 John Dallaway CLA 2010-05-07 10:19:51 EDT
Created attachment 167477 [details]
dsf-gdb.log

DSF GDB log - prompt when executing "exec-file"
Comment 33 John Dallaway CLA 2010-05-07 10:26:44 EDT
(In reply to comment #30)
> John, can you send me the gdb log from both the CDI and DSF sessions. The
> ordering of the load and exec-file commands are the same in both, since that
> code is in the common, default jtagdevice implementation. So, the fact that the
> order is fine for CDI but not DSF is an indication that there is some
> unintended difference between CDI and DSF elsewhere. The logs should help me
> figure what that difference is.

John, logs are now attached. Same i386-elf-gdb tool. Same executable file. The ordering of the relevant GDB commands seems to be identical and it appears that the executable file is no-longer passed to GDB as a command line parameter when using the CDI JTAG launcher. I can find no explanation for the difference in behaviour at present.
Comment 34 John Cortell CLA 2010-05-07 10:53:31 EDT
It looks to me CDI isn't complaining because the CDI does a -gdb-set confirm
off early on. So, in reality, it wants to ask us but we've told it to shut up
:-) So, looking more at load vs restore, and thinking about the history leading
up to this, it seems to me we should just be doing a load in all cases. The
reason restore was probably used instead of load was simply that the file was
being given to gdb at the cmdline. That effectively told gdb that's the file
that will be the focus of this debug session. It seems clear to me that the
load command processes the symbolics AND and marks that file as the one to
focus on. Restore simply does the former. As such, the use of restore made
sense back then. Now it doesn't. Using 'load' (in all cases) seems like exactly
what we need. Do you agree?
Comment 35 John Dallaway CLA 2010-05-07 11:53:22 EDT
(In reply to comment #34)
> Using 'load' (in all cases) seems like exactly what we need. Do you agree?

From a usability perspective, the 'load' command makes more sense for a generic remote debugging launcher which is intended to work for most people most of the time. The 'restore' command offers more flexibility, but users have plenty of flexibility via the "initialization commands" and "run commands" text boxes if they have special launch requirements.

The current implementation ('load' or 'restore' depending on image offset) is not at all obvious, but if you eliminate 'restore' behaviour in the default launcher implementation, you need to consider what happens to the "image offset" UI element. It should be disabled or deleted if its value is being ignored.
Comment 36 John Cortell CLA 2010-05-07 11:57:10 EDT
(In reply to comment #35)
> The current implementation ('load' or 'restore' depending on image offset) is
> not at all obvious, but if you eliminate 'restore' behaviour in the default
> launcher implementation, you need to consider what happens to the "image
> offset" UI element. It should be disabled or deleted if its value is being
> ignored.

The 'load' command supports specifying an offset
Comment 37 John Dallaway CLA 2010-05-07 12:07:15 EDT
(In reply to comment #36)
> The 'load' command supports specifying an offset

OK. In that case, using 'load' exclusively in the default implementation does indeed seem sensible.
Comment 38 John Cortell CLA 2010-05-07 12:11:23 EDT
(In reply to comment #37)
> (In reply to comment #36)
> > The 'load' command supports specifying an offset
> 
> OK. In that case, using 'load' exclusively in the default implementation does
> indeed seem sensible.

Done. Committed to HEAD. I think we can finally mark this one complete.
Comment 39 CDT Genie CLA 2010-07-28 15:26:01 EDT
*** cdt cvs genie on behalf of jcortell ***
[310304] GDB (DSF) Hardware Debugging Launcher ignores application program path

[*] CoreFileLaunchDelegate.java 1.36 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.launch/src/org/eclipse/cdt/launch/internal/CoreFileLaunchDelegate.java?root=Tools_Project&r1=1.35&r2=1.36
[*] LocalCDILaunchDelegate.java 1.12 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.launch/src/org/eclipse/cdt/launch/internal/LocalCDILaunchDelegate.java?root=Tools_Project&r1=1.11&r2=1.12
[*] LocalRunLaunchDelegate.java 1.16 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.launch/src/org/eclipse/cdt/launch/internal/LocalRunLaunchDelegate.java?root=Tools_Project&r1=1.15&r2=1.16
[*] LocalAttachLaunchDelegate.java 1.14 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.launch/src/org/eclipse/cdt/launch/internal/LocalAttachLaunchDelegate.java?root=Tools_Project&r1=1.13&r2=1.14

[*] AbstractCLaunchDelegate.java 1.68 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.launch/src/org/eclipse/cdt/launch/AbstractCLaunchDelegate.java?root=Tools_Project&r1=1.67&r2=1.68

[*] GDBJtagDebuggerTab.java 1.14 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/jtag/org.eclipse.cdt.debug.gdbjtag.ui/src/org/eclipse/cdt/debug/gdbjtag/ui/GDBJtagDebuggerTab.java?root=Tools_Project&r1=1.13&r2=1.14
[*] GDBJtagDSFDebuggerTab.java 1.8 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/jtag/org.eclipse.cdt.debug.gdbjtag.ui/src/org/eclipse/cdt/debug/gdbjtag/ui/GDBJtagDSFDebuggerTab.java?root=Tools_Project&r1=1.7&r2=1.8
[*] JtagUi.properties 1.6 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/jtag/org.eclipse.cdt.debug.gdbjtag.ui/src/org/eclipse/cdt/debug/gdbjtag/ui/JtagUi.properties?root=Tools_Project&r1=1.5&r2=1.6
[*] GDBJtagStartupTab.java 1.9 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/jtag/org.eclipse.cdt.debug.gdbjtag.ui/src/org/eclipse/cdt/debug/gdbjtag/ui/GDBJtagStartupTab.java?root=Tools_Project&r1=1.8&r2=1.9

[*] DebugCoreMessages.java 1.6 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.core/src/org/eclipse/cdt/debug/core/DebugCoreMessages.java?root=Tools_Project&r1=1.5&r2=1.6
[*] DebugCoreMessages.properties 1.13 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.core/src/org/eclipse/cdt/debug/core/DebugCoreMessages.properties?root=Tools_Project&r1=1.12&r2=1.13
[*] CDebugUtils.java 1.30 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.core/src/org/eclipse/cdt/debug/core/CDebugUtils.java?root=Tools_Project&r1=1.29&r2=1.30

[*] GDBJtagDebugger.java 1.10 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/jtag/org.eclipse.cdt.debug.gdbjtag.core/src/org/eclipse/cdt/debug/gdbjtag/core/GDBJtagDebugger.java?root=Tools_Project&r1=1.9&r2=1.10
[*] IGDBJtagConstants.java 1.6 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/jtag/org.eclipse.cdt.debug.gdbjtag.core/src/org/eclipse/cdt/debug/gdbjtag/core/IGDBJtagConstants.java?root=Tools_Project&r1=1.5&r2=1.6
[*] Message.properties 1.4 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/jtag/org.eclipse.cdt.debug.gdbjtag.core/src/org/eclipse/cdt/debug/gdbjtag/core/Message.properties?root=Tools_Project&r1=1.3&r2=1.4
[*] GDBJtagDSFFinalLaunchSequence.java 1.9 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/jtag/org.eclipse.cdt.debug.gdbjtag.core/src/org/eclipse/cdt/debug/gdbjtag/core/GDBJtagDSFFinalLaunchSequence.java?root=Tools_Project&r1=1.8&r2=1.9
[*] GDBJtagLaunchConfigurationDelegate.java 1.5 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/jtag/org.eclipse.cdt.debug.gdbjtag.core/src/org/eclipse/cdt/debug/gdbjtag/core/GDBJtagLaunchConfigurationDelegate.java?root=Tools_Project&r1=1.4&r2=1.5

[*] DefaultGDBJtagDeviceImpl.java 1.5 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/jtag/org.eclipse.cdt.debug.gdbjtag.core/src/org/eclipse/cdt/debug/gdbjtag/core/jtagdevice/DefaultGDBJtagDeviceImpl.java?root=Tools_Project&r1=1.4&r2=1.5