Bugzilla – Full Text Bug Listing
| Summary: | GDB Hardware Debugging - Memory viewing broken | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Evan Hunter <ehunter> | ||||
| Component: | cdt-debug | Assignee: | Nobody - feel free to take it <nobody> | ||||
| Status: | RESOLVED FIXED | QA Contact: | Doug Schaefer <cdtdoug> | ||||
| Severity: | major | ||||||
| Priority: | P3 | CC: | cdtdoug, marc.khouzam, nikolaj.liebrecht, nobody, pawel.1.piech, william.riley | ||||
| Version: | 8.2 | ||||||
| Target Milestone: | 8.2.1 | ||||||
| Hardware: | PC | ||||||
| OS: | Windows 7 | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
|
Description
Evan Hunter
I am also seeing this issue, using the GDB Hardware Debugging launch to connect to a local GDB server. The following error is logged when debugging the issue:
>!ENTRY org.eclipse.cdt.dsf 4 -1 2013-09-10 10:35:52.357
>!MESSAGE Uncaught exception in DSF executor thread
>!STACK 0
>java.lang.NullPointerException
> at org.eclipse.cdt.dsf.gdb.service.GDBMemory.isBigEndian(GDBMemory.java:197)
> at org.eclipse.cdt.dsf.gdb.service.GDBMemory$2.handleSuccess(GDBMemory.java:112)
> at org.eclipse.cdt.dsf.concurrent.RequestMonitor.handleCompleted(RequestMonitor.java:376)
> at org.eclipse.cdt.dsf.concurrent.RequestMonitor$2.run(RequestMonitor.java:303)
> at org.eclipse.cdt.dsf.concurrent.ImmediateExecutor.execute(ImmediateExecutor.java:63)
> at org.eclipse.cdt.dsf.concurrent.RequestMonitor.done(RequestMonitor.java:300)
> at org.eclipse.cdt.dsf.mi.service.MIMemory$2.handleSuccess(MIMemory.java:351)
> at org.eclipse.cdt.dsf.concurrent.RequestMonitor.handleCompleted(RequestMonitor.java:376)
> at org.eclipse.cdt.dsf.concurrent.RequestMonitor$2.run(RequestMonitor.java:303)
> at org.eclipse.cdt.dsf.concurrent.DefaultDsfExecutor$TracingWrapperRunnable.run(DefaultDsfExecutor.java:374)
> at java.util.concurrent.Executors$RunnableAdapter.call(Unknown Source)
> at java.util.concurrent.FutureTask$Sync.innerRun(Unknown Source)
> at java.util.concurrent.FutureTask.run(Unknown Source)
> at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$201(Unknown Source)
> at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(Unknown Source)
> at java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source)
> at java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)
> at java.lang.Thread.run(Unknown Source)
It appears initializeMemoryData in GDBMemory is never being called during a GDB Hardware Debugging session, resulting in the NPE when a Memory Block is retrieved.
Created attachment 235356 [details]
Patch to initialize GDBMemory during launch
This patch calls initializeMemoryData as step during the launch& start/restart process sequences. stepInitializeMemory is taken from DebugNewProcessSequence.
I am not familiar with the Hardware Debugging launcher and may be missing something but isn't it sufficient to add the memory initialization step to GDBJtagDSFFinalLaunchSequence only? The memory initialization needs to be done just once at the beginning of the session. (In reply to Mikhail Khodjaiants from comment #3) > I am not familiar with the Hardware Debugging launcher and may be missing > something but isn't it sufficient to add the memory initialization step to > GDBJtagDSFFinalLaunchSequence only? The memory initialization needs to be > done just once at the beginning of the session. It might be sufficient and it does fix the issue (the NPE will only occur if it is never called). However from looking at initializeMemoryData and how it is called from GDBProcesses.attachDebuggerToProcess it appears it should be called for each IMemoryDMContext so I thought adding it to the start/restart sequence as well was safer. I couldn't see any obvious problems from calling it there in addition to the launch sequence, although I have only tested with GDB Hardware Debugging. (In reply to William Riley from comment #4) > (In reply to Mikhail Khodjaiants from comment #3) > > I am not familiar with the Hardware Debugging launcher and may be missing > > something but isn't it sufficient to add the memory initialization step to > > GDBJtagDSFFinalLaunchSequence only? The memory initialization needs to be > > done just once at the beginning of the session. > > It might be sufficient and it does fix the issue (the NPE will only occur if > it is never called). However from looking at initializeMemoryData and how it > is called from GDBProcesses.attachDebuggerToProcess it appears it should be > called for each IMemoryDMContext so I thought adding it to the start/restart > sequence as well was safer. I couldn't see any obvious problems from calling > it there in addition to the launch sequence, although I have only tested > with GDB Hardware Debugging. The Hardware Debugging uses the start/restart sequence for restart only. In this case the memory context is the same, there is no need to initialize memory data. William, You need to sign the CLA otherwise I can't check in the patch. Please do it before Tuesday, Sep 17. See http://wiki.eclipse.org/Development_Resources/Contributing_via_Git#Eclipse_Foundation_Contributor_License_Agreement. Thanks (In reply to Mikhail Khodjaiants from comment #6) > William, > > You need to sign the CLA otherwise I can't check in the patch. Please do it > before Tuesday, Sep 17. See > http://wiki.eclipse.org/Development_Resources/ > Contributing_via_Git#Eclipse_Foundation_Contributor_License_Agreement. > > Thanks Apologies, I didn't realise this was required for patches now. I have signed the agreement now. Checked in the GDBJtagDSFFinalLaunchSequence part of the patch to 8.2 and master branches. Thank you. *** cdt git genie on behalf of William Riley ***
Bug 413483 - GDB Hardware Debugging - Memory viewing broken
Signed-off-by: William Riley <william.riley@xxxxxxxxxxx>
[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=b242f8eb38d4a88495d90508b5ce74f542b57eba
*** cdt git genie on behalf of William Riley ***
Bug 413483 - GDB Hardware Debugging - Memory viewing broken
Signed-off-by: William Riley <william.riley@xxxxxxxxxxx>
[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=72925ad5b4c0556de8ebe4cc5926a383a467226e
(In reply to Mikhail Khodjaiants from comment #8) > Checked in the GDBJtagDSFFinalLaunchSequence part of the patch to 8.2 and > master branches. The change to 8_2 cause an API error because a new API was added. I guess adding an API filter is the simplest way to go. If we really don't want to add a new API (stepInitializeMemory()), we could add the code of stepInitializeMemory() directly into stepUpdateContainer() which is run right before (not pretty but that code will die with 8_2). Either way is fine with me. *** cdt git genie on behalf of Mikhail Khodjaiants ***
Bug 413483 - GDB Hardware Debugging - Memory viewing broken
To avoid introducing a new API in 8.2.1 the memory initialization is
added to "stepUpdateContainer".
[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=8fd76f3f548e23e4a360da464911d17ccd8a1272
(In reply to Marc Khouzam from comment #11) > (In reply to Mikhail Khodjaiants from comment #8) > > Checked in the GDBJtagDSFFinalLaunchSequence part of the patch to 8.2 and > > master branches. > > The change to 8_2 cause an API error because a new API was added. I guess > adding an API filter is the simplest way to go. > > If we really don't want to add a new API (stepInitializeMemory()), we could > add the code of stepInitializeMemory() directly into stepUpdateContainer() > which is run right before (not pretty but that code will die with 8_2). > > Either way is fine with me. Thanks Marc, I decided to go with the second option instead of adding an API filter and checked the fix in. If you have a chance please take a look. BTW, how do you get the API error? Are you using a new API baseline for 8.2.1? (In reply to Mikhail Khodjaiants from comment #13) > I decided to go with the second option instead of adding an API filter and > checked the fix in. If you have a chance please take a look. Since the call to setContainerContext() is synchronous, you could call the memory stuff right after it, which makes it much simpler. Like this: @Execute public void stepUpdateContainer(RequestMonitor rm) { String groupId = getContainerContext().getGroupId(); setContainerContext(fProcService.createContainerContextFromGroupId( fCommandControl.getContext(), groupId)); // Initialize memory in this step to avoid adding API IGDBMemory memory = fTracker.getService(IGDBMemory.class); IMemoryDMContext memContext = DMContexts.getAncestorOfType(getContainerContext(), IMemoryDMContext.class); if (memory == null || memContext == null) { rm.done(); return; } memory.initializeMemoryData(memContext, rm); } > BTW, how do you get the API error? Are you using a new API baseline for > 8.2.1? I didn't get the error, but I thought I should. It was Marc-Andre that realized I didn't have the GDB Hardware Debugging feature as part of my API baseline. So, I launched the Eclipse I used as API baseline and installed every CDT feature available. Then I went back to my dev eclipse, Edited the baseline and pressed Reset to refresh it. Then the error appeared. To make sure all plugins get installed Marc-Andre told me I should uncheck the box "group by feature" because apparently, plugins that are not part of a feature won't be visible when using "group by feature". (In reply to Marc Khouzam from comment #14) > (In reply to Mikhail Khodjaiants from comment #13) > > > I decided to go with the second option instead of adding an API filter and > > checked the fix in. If you have a chance please take a look. > > Since the call to setContainerContext() is synchronous, you could call the > memory stuff right after it, which makes it much simpler. Like this: > > @Execute > public void stepUpdateContainer(RequestMonitor rm) { > String groupId = getContainerContext().getGroupId(); > setContainerContext(fProcService.createContainerContextFromGroupId( > fCommandControl.getContext(), groupId)); > > // Initialize memory in this step to avoid adding API > IGDBMemory memory = fTracker.getService(IGDBMemory.class); > IMemoryDMContext memContext = > DMContexts.getAncestorOfType(getContainerContext(), IMemoryDMContext.class); > if (memory == null || memContext == null) { > rm.done(); > return; > } > memory.initializeMemoryData(memContext, rm); > } > > > BTW, how do you get the API error? Are you using a new API baseline for > > 8.2.1? > Thanks, I changed the code. *** cdt git genie on behalf of Mikhail Khodjaiants ***
Bug 413483 - GDB Hardware Debugging - Memory viewing broken
The memory initialization is moved to "stepUpdateContainer" to avoid
introducing a new API in 8.2.1.
[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=82409f57c0cce9be3946b7d9d233d265d279d52f
(In reply to Mikhail Khodjaiants from comment #15) > Thanks, I changed the code. Thanks for taking care of that. I should have thought about the Hardware debug during my review... I was thinking that for safety, we should protect against the NPE, in case tehre is another use-case where we forgot to call initializeMemoryData(). What do you think about changing, in GDBMemory: from @Override public boolean isBigEndian(IMemoryDMContext context) { return fIsBigEndian; } to @Override public boolean isBigEndian(IMemoryDMContext context) { return fIsBigEndian == null ? false : fIsBigEndian; } I'd rather have the wrong endianness than the NPE. (In reply to Marc Khouzam from comment #17) > (In reply to Mikhail Khodjaiants from comment #15) > > > Thanks, I changed the code. > > Thanks for taking care of that. I should have thought about the Hardware > debug during my review... > > I was thinking that for safety, we should protect against the NPE, in case > tehre is another use-case where we forgot to call initializeMemoryData(). > > What do you think about changing, in GDBMemory: > > from > @Override > public boolean isBigEndian(IMemoryDMContext context) { > return fIsBigEndian; > } > to > @Override > public boolean isBigEndian(IMemoryDMContext context) { > return fIsBigEndian == null ? false : fIsBigEndian; > } > > I'd rather have the wrong endianness than the NPE. I have no objection. I also think we need to log a message if the memory is not initialized properly. That would help us to diagnose the problem earlier. (In reply to Mikhail Khodjaiants from comment #18) > I have no objection. I also think we need to log a message if the memory is > not initialized properly. That would help us to diagnose the problem earlier. Good point. I'll push something to Gerrit. What do you think about trying to squeeze it in into today's 8_2 release? I pushed the update for master to Gerrit: https://git.eclipse.org/r/16507 I put in an assert for us to notice more easily, but only put a log for production code, so that there would be no NPE. Mikhail, what do you think? Should I put it in 8_2? (In reply to Marc Khouzam from comment #20) > I pushed the update for master to Gerrit: > https://git.eclipse.org/r/16507 > > I put in an assert for us to notice more easily, but only put a log for > production code, so that there would be no NPE. > > Mikhail, what do you think? Should I put it in 8_2? Yes, if we still can do it, putting it in 8_2 would be good. I think we should also a similar check for the address size. *** cdt git genie on behalf of Marc Khouzam ***
Bug 413483 - Protect agains NPE if endianness is not initialized
Change-Id: I8ab096dacb9d2fcf0d261dd082fa0fdc84485219
Signed-off-by: Marc Khouzam <marc.khouzam@xxxxxxxxxxxx>
[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=91d183efc2a77a6efc1bbe6129a00f7254a7dca8
(In reply to Mikhail Khodjaiants from comment #21) > Yes, if we still can do it, putting it in 8_2 would be good. I put the fix in 8_2: http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?h=cdt_8_2&id=91d183efc2a77a6efc1bbe6129a00f7254a7dca8 > I think we should also a similar check for the address size. I looked at the address size case and there is no risk of NPE because we allocate the HashMap at construction time. I guess we could add a log but since I believe the endianness is checked just as often, we will get the log entry anyway. (In reply to Marc Khouzam from comment #23) > (In reply to Mikhail Khodjaiants from comment #21) > > > Yes, if we still can do it, putting it in 8_2 would be good. > > I put the fix in 8_2 but I had forgotten to put it in master, which I just did, from Gerrit. |