Review Board 2.0.15


imported patch ext/arm_gdb.patch

Review Request #293 - Created Nov. 8, 2010 and submitted

Information
Ali Saidi
gem5
Reviewers
Default 
imported patch ext/arm_gdb.patch

   

  


 


 

  

  









  
 


  

 


















 

  


 


  



   

   
Nathan Binkert

   
Posted  (Nov. 8, 2010, 10 a.m.)


  


  


  

   
Was the remote_gdb.cc file copied with "hg cp"  Be nice if it were.



  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    I'll make sure the committed code does that. Any other issues?
    
    
    
   
    2. 

   


  





  

 


  

 



















 

  


 


  




   
Ship it!



   

   
Nathan Binkert

   
Posted  (Nov. 8, 2010, 10:54 p.m.)


  


  


  

   



  

   

   
    
    
   


  





  

 


  

 

















 



 

  


 


  



   

   
Gabe Black

   
Posted  (Nov. 9, 2010, 3:50 a.m.)


  


  


  

   
Instead of packing/unpacking 32 bit values in 64 bit registers, why don't we template the base class on the type of register it's going to hold. I think the changes to the base class would be minimal, and it would simplify this ARM code nicely with almost no impact on the other ISAs.



  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    I sort of agree, but I don't think it's worth the time. The remote gdb code is already a disaster area and putting a bandaid on it doesn't really help much. It was quite a pain to make sure this was correct and test every register, so I'd prefer to leave it this way until some overall restructuring of the remote gdb code occurs. 
    
    
    
   
    2. 


   
  2. 
    
    
     
    
      
     
    
     
    Ok. Maybe we should add a bug to flyspray (and start actually using flyspray), and/or start talking about what needs to happen on m5-dev. I originally split up the remote gdb code from its monolithic Alpha implementation, and it was a big pain since the GDB protocol is a big pain. It's one of those things that just sort of goes off the rails rather than complaining about anything specific. I don't think the split is horrifically bad, but I also don't doubt it can be improved. I think there may be some functional issues too with breakpoints, although maybe that was specific to a particular ISA (Alpha, I think which was concerning) or maybe I wasn't using it right. It would be great to have some testing for this. This and checkpoints are just asking to bit rot since there's no testing and I don't even know how to use checkpointing.
    
    
    
   
    3. 

   


  




   







    

     
     

      

       
        

         
          src/arch/arm/remote_gdb.hh
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
If you're not going to indent the constants above (which seems to be the preferred way to do things) then you should unindent this. I think it's better to be consistent that inconsistently correct.




  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Fixed.
    
    
    
   
    2. 

   


  


    


    

     
     

      

       
        

         
          src/arch/arm/remote_gdb.cc
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
This is already happening in the base class constructor.




  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Fixed.
    
    
    
   
    2.