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

Bug 353904

Summary: Scope does not correctly compute whether the range is empty
Product: [Tools] CDT Reporter: Daniel Thomas <daniel.thomas>
Component: cdt-debug-edcAssignee: Ken Ryall <ken.ryall>
Status: RESOLVED FIXED QA Contact: Ken Ryall <ken.ryall>
Severity: normal    
Priority: P3 CC: cdtdoug, kirk.beitz
Version: 8.0   
Target Milestone: ---   
Hardware: All   
OS: All   
Whiteboard:
Attachments:
Description Flags
return true if lowAddress.equals(highAddress) cdtdoug: iplog+

Description Daniel Thomas CLA 2011-08-04 11:17:31 EDT 
Build Identifier: 

Scope only returns true from hasEmptyRange() when either both low and high addresses are null, or zero or low is -1 and high is 0. It does not return true if the low address is equal to the high address which is the common case for an empty range.

This causes tests attached to https://bugs.eclipse.org/bugs/show_bug.cgi?id=353577 to fail.

Reproducible: Always
Comment 1 Daniel Thomas CLA 2011-08-04 11:18:40 EDT 
Created attachment 200924 [details]
return true if lowAddress.equals(highAddress)
Comment 2 Kirk Beitz CLA 2011-08-05 15:42:57 EDT 
patch applied and nominally tested locally; awaiting final review and final
merge to eclipse edc git repository by committer ken ryall.
Comment 3 Kirk Beitz CLA 2011-08-06 17:35:11 EDT 
Daniel,

Even with this patch merged to the source, it would be nice to have a JUnit test case that exposes this problem if the patch is not applied.

This is another patch that i'm afraid may cause us regressions with some of our compilers' symbol generation quirks, and want to be able to accommodate both the needs you are attempting to address with this patch and any regressions this may cause.

Do you have a small executable that exhibits this problem?  Or perhaps you could generate a snapshot to add to the edc test resources that demonstrates the problem and could be tested with a new JUnit test?
Comment 4 Daniel Thomas CLA 2011-08-08 07:12:53 EDT 
(In reply to comment #3)
> Daniel,
> 
> Even with this patch merged to the source, it would be nice to have a JUnit
> test case that exposes this problem if the patch is not applied.

https://bugs.eclipse.org/bugs/attachment.cgi?id=200917 tests this functionality and writing that test was what caused me to notice the bug. It is a bug  that does cause problems when examining executables we have (3 occurrences of this causing an incorrect return value from hasEmptyRange on the executable I tried)

> This is another patch that i'm afraid may cause us regressions with some of our
> compilers' symbol generation quirks, and want to be able to accommodate both
> the needs you are attempting to address with this patch and any regressions
> this may cause.
> 
> Do you have a small executable that exhibits this problem?  Or perhaps you
> could generate a snapshot to add to the edc test resources that demonstrates
> the problem and could be tested with a new JUnit test?

I don't have such an executable that could be submitted. I would have to make one from scratch.

This patch shouldn't cause any problems if the code which sets the Scope high and low addresses is correct as if the low and high addresses are equal then the range covered by the scope is 0 and so the Scope has an empty range.
It would have to be quirks in the compiler's debug information generation for this to cause problems.
Comment 5 Kirk Beitz CLA 2011-08-08 14:43:29 EDT 
(In reply to comment #4)
> This patch shouldn't cause any problems if the code which sets the Scope high
> and low addresses is correct as if the low and high addresses are equal then
> the range covered by the scope is 0 and so the Scope has an empty range.
> It would have to be quirks in the compiler's debug information generation for
> this to cause problems.

indeed, this is exactly the problem we face:  quirks in the compiler's debug information generation, and a frozen compiler source base, where no changes to the debug information generation will be made.

i even seem to recall one such quirk where high addresses were less than low addresses ... though not in the case of scope ranges specifically (i think it was a line-number table item).

in any case, you wouldn't have to submit an executable, but merely a snapshot that would show the bug as exposed without your patch, and fixed with it.  and then if we run into any such quirks ourselves that we have to workaround in the scope range code, we can regression test with your snapshot to make certain we don't break your case.
Comment 6 Daniel Thomas CLA 2011-08-09 09:04:11 EDT 
I would need to produce an executable as though I am using EDC for debugging I am using its internal APIs to analyze executables and so I can't produce a snapshot.
Should I work out how to do that?
Comment 7 Kirk Beitz CLA 2011-08-09 12:48:04 EDT 
(In reply to comment #6)
> I would need to produce an executable as though I am using EDC for debugging I
> am using its internal APIs to analyze executables and so I can't produce a
> snapshot.
> Should I work out how to do that?

in that case (since the point of my request for a regression test is to make certain we don't break your symbolics reading scenario should we find a need to tweak this patch), an appropriate JUnit test wouldn't need a snapshot necessarily, but would approximates your steps of analysis that fails without your patch, but works fine with it.
Comment 8 Daniel Thomas CLA 2011-08-12 07:06:09 EDT 
(In reply to comment #7)
> (In reply to comment #6)
> > I would need to produce an executable as though I am using EDC for debugging I
> > am using its internal APIs to analyze executables and so I can't produce a
> > snapshot.
> > Should I work out how to do that?
> 
> in that case (since the point of my request for a regression test is to make
> certain we don't break your symbolics reading scenario should we find a need to
> tweak this patch), an appropriate JUnit test wouldn't need a snapshot
> necessarily, but would approximates your steps of analysis that fails without
> your patch, but works fine with it.

In addition to the test in https://bugs.eclipse.org/bugs/show_bug.cgi?id=353577 which fails without this patch then the following:
@Test
	public void testHasEmptyRangeExclusion(){
		IAddressFactory addrFact = new Addr64Factory();
		Scope parent = new FunctionScope("parent", null, addrFact.getZero(), addrFact.createAddress("0x200"), null);
		
		IScope emptyA = new LexicalBlockScope("emptyA", parent, addrFact.getZero(), addrFact.getZero());
		parent.addChild(emptyA);
		IScope twenty = new LexicalBlockScope("twenty", parent, addrFact.getZero(),addrFact.createAddress("0x20"));
		parent.addChild(twenty);
		IScope thrity_fourty = new LexicalBlockScope("thrity_fourty", parent, addrFact.createAddress("0x30"),addrFact.createAddress("0x40"));
		parent.addChild(thrity_fourty);
		IScope emptyB = new LexicalBlockScope("emptyB", parent, addrFact.createAddress("0x40"), addrFact.createAddress("0x40"));
		parent.addChild(emptyB);
		IScope fifty_hundrend = new LexicalBlockScope("fifty_hundrend", parent, addrFact.createAddress("0x50"),addrFact.createAddress("0x100"));
		parent.addChild(fifty_hundrend);
		IScope emptyC = new LexicalBlockScope("emptyC", parent, addrFact.createAddress("0x120"), addrFact.createAddress("0x120"));
		parent.addChild(emptyC);
		
		List<IScope> cleanChildren = new ArrayList<IScope>(3);
		for (IScope child : parent.getChildren()){
			if (!child.hasEmptyRange()){
				cleanChildren.add(child);
			}
		}
		assertEquals("Correct number of children",3,cleanChildren.size());
	}
Also exercises that failure mode (not sure where to put that test as your current tree has diverged from upstream and mine has diverged differently). It emulates the logic used in my code which fails if hasEmptyRange doesn't actually test for an empty range.

I would hope that bugs in the compiler could be dealt with inside the DwarfInfoReader etc. and not continue to cause problems with incorrect information propagating through the rest of EDC (though unfortunately it may be that not all bugs can be fixed up).