Review Board 2.0.15


ruby: get rid of ruby's Debug.hh

Review Request #367 - Created Jan. 4, 2011 and submitted

Information
Nathan Binkert
gem5
Reviewers
Default
ali, gblack, nate, stever 
ruby: get rid of ruby's Debug.hh

Get rid of the Debug class
Get rid of ASSERT and use assert
Use DPRINTF for ProtocolTrace
This compiles and passes all of the quick regressions, but it would be nice for a Ruby developer to take a look and see if I got rid of any useful functionality.
Review request changed
Updated (Jan. 4, 2011, 7:02 a.m.)

Diff:

Revision 3 (+115 -720)

Show changes

configs/ruby/Ruby.py
src/mem/SConscript
src/mem/ruby/buffers/MessageBuffer.hh
src/mem/ruby/buffers/MessageBuffer.cc
src/mem/ruby/common/Debug.py
src/mem/ruby/common/Debug.hh
src/mem/ruby/common/Debug.cc
src/mem/ruby/common/Global.hh
27 more
Brad Beckmann
Posted (Jan. 4, 2011, 8:31 a.m.)
Hi Nate,

I have a couple questions:

1. Have you looked at the protocol trace output after your change?  Does it look exactly like it did before?  It seems that the output should be the same based on my brief inspection of your patch, but I would like to be sure about that.  It may not be obvious, but there is a specific rational behind the format of the protocol trace and I want to make sure that stays the same.

2. With your patch applied, what happens if one hits an assert when running interactively?  Previously, the process would abort allowing one to attach gdb and examine what is going on.  I liked that feature and it would be great if we could maintain it.  Could we port that feature to all of M5?
  1. 1) I have not, because I don't know how, but I tried hard to make it exactly the same.  Can you help me out?  It won't look identical because DPRINTF prepends some stuff (curTick and object name)

2) we don't have a mechanism to have the process stall until GDB is attached, but given that this worked in Ruby only, I'd agree that this should be something that we do globally in M5.  The right way to do this would be to handle SIGABRT and stall in the abort handler (I think that should work).  Can we work on this patch and do that as a separate one?
  2. Brad, do you have some protocol trace with you? I have seen the trace that gets generated with the current trace facility using Ruby trace flag. It prints all the events for all the cache controllers and network routers. If you prefer, I can send you an example trace. Or you can generate one by running m5.opt with trace file and trace flag options supplied.

./build/ALPHA_SE_MESI_CMP_directory/m5.opt  --trace-file=MESI.trace  --trace-flags=Ruby ./configs/example/ruby_random_test.py -l 1000
Ali Saidi
Posted (Jan. 4, 2011, 8:48 a.m.)



  

   

   
    
    
   


  




   







    

     
     

      

       
        

         
          src/mem/ruby/buffers/MessageBuffer.cc
          

           (Diff revision 3)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
Why continue to comment this out?




  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Mostly because I didn't do it.  I don't like commented code in the tree, and one of the reasons I don't like it is because I don't know why it was commented out.  Should I remove it?
    
    
    
   
    2. 

   


  


    


    

     
     

      

       
        

         
          src/mem/ruby/common/Global.cc
          

           (Diff revision 3)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
This doesn't seem like a applicable change, also aren't they equivalent?




  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Heh, I just had a discussion about this sort of thing with Gabe.  NULL is a C thing.  In C++, the standard says to use 0 for pointers.  In this specific case, I'd have to add a #include to bring in the #define for NULL.  Seems like a bad idea to me, no?
    
    
    
   
    2. 

   


  


    


   



  

 


  

 

















 



 

  


 


  



   

   
Nilay Vaish

   
Posted  (Jan. 4, 2011, 11:17 a.m.)


  


  


  

   



  

   

   
    
    
   


  




   







    

     
     
    

    


     
Should this not be converted to DPRINTF()?




  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Perhaps, but that is really a separate change.  I don't know what flag to base it on.  I only changed from printf to cprintf so I wouldn't have to #include <cstdio>
    
    
    
   
    2.