Bug 321186

Comment 1 Kirk Beitz 2010-08-01 16:55:59 EDT

Created attachment 175672 [details]
changes that once again allow variable-change highlighting to work

Ken Ryall noted in a separate email:

> It looks like variable highlighting broke with the changes in Stack.java
> between version 1.19 and 1.20 (Eclipse CVS). Ling’s comments from 5/26/10 are:
> 
> added proper implementation of equal() and hashCode() in StackFrameDMC, fixed
> the bug that stale stack frame was used in variable view refresh and wrong
> variables were displayed.

the problem that occurs is that from step to step or breakpoint to breakpoint, the ipAddress will change (and to avoid the confusion i encountered when noticing this, ipAddress refers not to an IP address, but rather to an instruction pointer address).  this causes the hash-code to be different at every generation, and causes comparison in equal() to fail

by modifying the portion of equal() and hashCode() not to rely on ipAddress, change highlighting is restored.  the hashCode now generated uses all of the other elements previously added (baseAddress, executionDMC, and level), and comparison also is based upon pre-1.20 critera plus only baseAddress, executionDMC and level comparison and not ipAddress, and these changes allow the comparison necessary to allow highlighting to take place.

the attachment Stack.java.1.24.patch contains my proposed changes:

- the fixes described in the preceding paragraph

- refactoring all references to ipAddr to instead be instrPtrAddr (so it is more clear to the casual observer exactly what the variable refers to)

this may not be ultimately the best way to address the stack comparison that must take place to determine whether or not a variable change has occurred, but at least for now, it will restore the variable change functionality.

Comment 2 Kirk Beitz 2010-08-02 15:09:58 EDT

Created attachment 175728 [details]
more complete patch of all files touched by re-factoring

ipAddress -> instrPtrAddress refactoring touched public members referenced in other files; the latest attachment is a patch that reflects the more complete nature of this patch.

(Ling Wang has recently updated RunControl.java to 1.27 ... which means the patch may have to be regenerated to apply cleanly to head, since the version this patch applies to is 1.24 .)

Comment 3 Kirk Beitz 2010-08-02 15:28:41 EDT

Created attachment 175729 [details]
more complete patch of all files touched by re-factoring

Comment 4 Kirk Beitz 2010-08-05 03:25:38 EDT

Created attachment 175907 [details]
combined patch, tested using modified blackflag containing with lexical scopes

combined.321186.patch is a replacement for all other patches here, and fixes
- the problem of variable-change highlighting of this bugzilla issue
- the problem of variables not being seen at times or at all that ling
  was trying to fix in Stack.java rev 1.20 (no eclipse bugzilla number)
- a combination problem of scoped variables being shown beyond their lifetime


detailed rationale for changes in the patch:

  [1] removing instr ptr addr from equal() & hash() functions

      this is the guts of the 321186 fix, once again allowing StackFrameDMC
      comparisons that result in stack frames being considered equal, and thus
      allowing variables to once again be properly highlighted when changed.
      [file: Stack.java]



  [2] new IAddress and boolean "trustCache" parameters to getLocals()

      the guts of the fix that ling was trying to make in Stack.java 1.20
      that caused the regression resulting in 321186 ... allowing both
      variable change highlighting AND all local variables, including scoped
      variables or variables with additional symbolic lifetime information, to
      be displayed only at the appropriate times.

      - half of the problem was the instr ptr addr held by the StackFrameDMC
        got stale; passing the IAddress to getLocals() fixes this part, since
        the proper instr ptr address is available when retrieving frames are
        retrieved.

      - the other half of the problem was a conditional in getLocals() was
        preventing the cache of locals from being purged/refreshed; the new
        trustCache parameter more clearly helps the caller decide whether
        the cache should be trusted as is, or flushed and re-fetched.

      [files: Stack.java, RunControl.java]



  [3] "external" get locals compares frameContext with new frames

      in Stack.java, the caller the getLocals() in [2] above is another
      function named getLocals(). the new code finds the frame in the new
      stack that corresponds to the one passed in (based upon the equals()
      in [1] above).  if equal but containing differing instr ptr addrs,
      the trustCache parameter to getLocals() is false, and additionally, the 
      stale instr ptr addr is updated, and. if equal with identical instr ptr
      addrs, the getLocals(,trustCache) parameter is passed as true.  (some
      thought was given to not having an address parameter and using the
      updated (formerly stale) instr ptr address as the sole means of
      conveying the new value into the "inner" getLocals() ; the choice was
      made to add it for potential future usage.)

      [file: Stack.java]



  [4] different bounds within FunctionScope#recurseGetScopedVariables()

      scoped variables were appearing beyond the brackets of their scope due
      to the original tests of this function; fixed by adjusting the top check 
      to be "<" instead of "<=" ... except when the scope being examined is
      a function scope or the outermost scope, allowing variables in function
      to be seen all the way to the final bracket.

      [file: FunctionScope.java]


  [5] ipAddress -> instrPtrAddress , IP_ADDR -> INSTR_PTR_ADDR

      ipAddress generally means "internet protocol"; was confusing in
      the case of tracking down the original equals()/hash() problems

      [files: EvaluateID.java, VariableWithValue.java, RunControl.java,
              Snapshot.java, MemoryVariableLocation.java,
              DwarfFrameRegisterProvider.java, DwarfFrameRegisters.java,
              Stack.java, ARMStack.java, X86Stack.java]

  [6] new address(), updateInstrAddrPtr(), getTopFrame() in Stack.java
      readability refactoring, allowing use by both internal and rm calls

      [file: Stack.java]

Comment 5 Ken Ryall 2010-08-06 00:12:15 EDT

After I applied the patch the TestDwarfReader.testLocalScopes1 fails:

java.lang.AssertionError: BlackFlag_gcce.sym:dbg_pointers.cpp:ptrToArray:10854: found extra variables: stackArray, pstackArray, value, pheapArray, objArray, pobj, 
BlackFlag_gcce.sym:dbg_pointers.cpp:ptrToArray:10a2a: missing m, 
BlackFlag_gcce.sym:dbg_pointers.cpp:ptrToArray:10a2c: found extra variables: stackArray, pstackArray, value, pheapArray, objArray, pobj, 
BlackFlag_rvct.sym:dbg_linked_lists.cpp:list:find:a652: found extra variables: aux, 
BlackFlag_rvct.sym:dbg_linked_lists.cpp:list:find:a656: found extra variables: aux, 
SimpleCpp_gcc_x86.exe:Templates.cpp:List:add:80487a6: found extra variables: newmax, copy, 
SimpleCpp_gcc_x86.exe:Templates.cpp:List:add:8048855: found extra variables: newmax, copy, 

	at org.junit.Assert.fail(Assert.java:91)
	at org.eclipse.cdt.debug.edc.tests.TestDwarfReader.testLocalScopes1(TestDwarfReader.java:1963)

Comment 6 Kirk Beitz 2010-08-06 03:12:52 EDT

(In reply to comment #5)
> After I applied the patch the TestDwarfReader.testLocalScopes1 fails:
>     at org.junit.Assert.fail(Assert.java:91)
>     at org.eclipse.cdt.debug.edc.tests.TestDwarfReader.testLocalScopes1(TestDwarfReader.java:1963)

ken,

i'll address each of the failures reported in your comment separately below.  

the summary is that a couple appear to be failures in the JUnit tests themselves, but at least one is probably a failure with getting scopes right in a C++ template that needs to be debugged and re-patched.

i need to follow up on the one probable failure by re-building that executable for debugging, and once i've examined and re-created the patch for that, i need to also create patches for the JUnit tests that i think are incorrect ... if there is agreement about them being incorrect.

----

details:

first, context for referring to several of the problems below:

{fs} - will refer to the patch to FunctionScope.cpp; it has two parts: [1] the
       part of the patch changing "<= 0" to "< 0" for the uppermost comparison,
       which is meant to make some of the variables that i saw appearing at the
       instruction after a closing scope brace; [2] the part of the patch which
       gets the locals for a "kid" when the current scope is a function level
       scope ... which often was preventing the user from seeing the function
       variables at the last brace ... meaning the user would step past the
       last line, and then be unable to see the variables even though still in
       the function.

       i'll reference {fs} to refer to the cases below affected by this change.


> java.lang.AssertionError: BlackFlag_gcce.sym:dbg_pointers.cpp:ptrToArray:10854:
> found extra variables: stackArray, pstackArray, value, pheapArray, objArray,
> pobj, 

{fs} these variables are all function-scope variables, and this "assertion" occurs on the first address of the function.  i'm pretty certain it's ok to see all of these variables at that address; the tester probably noted that no variables were visible and assumed none should be shown.  so i think the JUnit test is wrong in this case.


> BlackFlag_gcce.sym:dbg_pointers.cpp:ptrToArray:10a2a: missing m, 

{fs} in this case, m is a loop variable; in my testing, i see m everywhere i'm supposed to see m, so i think it may be possible that this is also a JUnit failure with regard to m and its final brace ... particularly having stepped through the JUnit in the debugger and seeing where this "assert" occurs.


> BlackFlag_gcce.sym:dbg_pointers.cpp:ptrToArray:10a2c: found extra variables:
> stackArray, pstackArray, value, pheapArray, objArray, pobj, 

{fs} similar to the extra six seen above, except these are on the last instruction; i think the test is wrong for this.



> BlackFlag_rvct.sym:dbg_linked_lists.cpp:list:find:a652: found extra variables:
> aux, 
> BlackFlag_rvct.sym:dbg_linked_lists.cpp:list:find:a656: found extra variables:
> aux, 

unfortunately, there's some missing context for this case:  the file dbg_linked_lists.cpp that was used to build BlackFlag_rvct.sym referred to here must no longer be the same, because none of the variables in the file appear when stepping carefully through the JUnit test, and none of the variables in the test appear in the file.

it seems possible the {fs} change regarding variables one level below functionScope is interfering based upon stepping through the code ... but without the original source to build for further testing, it's hard to know ...



> SimpleCpp_gcc_x86.exe:Templates.cpp:List:add:80487a6: found extra variables:
> newmax, copy, 
> SimpleCpp_gcc_x86.exe:Templates.cpp:List:add:8048855: found extra variables:
> newmax, copy, 

the source for this does appear to be up to date, and the variables mentioned seem to exist outside the bounds of the conditional scope that they appear in within a template.

Comment 7 Kirk Beitz 2010-08-06 17:11:46 EDT

finally figured out what the "enabled != showAllVariablesEnabled" test was for.

the patch also breaks the Variable View menu "Show Globals Defined in File".

seems perhaps it would make more sense if the menu was named something like showGlobalsDefinedInFile, and if the preference was also named something similar, rather than SHOW_ALL_VARIABLES_ENABLED.

in any case, the updated patch will have to include this as well as the fixes uncovered by the JUnit testing.

Comment 8 Kirk Beitz 2010-08-07 13:55:32 EDT

Created attachment 176094 [details]
pruned patch that fixes only the variable change highlighting

Stack.java.321186.patch reflects the bare minimum of changes necessary to fix the regression caused in attempting to fix viewing of variables in specific scopes (cf. https://bugs.eclipse.org/bugs/show_bug.cgi?id=322051)

fixes in patch Stack.java.321186.patch
- remove use of ipAddress as part of hash() and in comparison as part of equals()
- change to outer getLocals() to update stale StackFrameDMC ipAddress
- change to inner getLocals() to accept "trustCache" boolean, and if it is false,
  then get the locals from scratch

Stack.java.321186.patch obsoletes all other patches.

Comment 9 Ken Ryall 2010-08-08 08:26:41 EDT

Committed patch to HEAD after a couple minor adjustments.

Comment 10 Kirk Beitz 2010-08-08 14:10:18 EDT

Created attachment 176118 [details]
to go with the name change from trustCache to useVariableCache

i see one of the minor changes you made was to change the parameter name from "trustCache" to "useVariablesCache".  to that end, i suggest also committing the Stack.java.1.27.patch (attached), which performs the same name refactoring in function getLocals(IFrameDMContext, DataRequestMonitor<IVariableDMContext[]>) .