Review Board 2.0.15


trace: reimplement the DTRACE function so it doesn't use a vector

Review Request #352 - Created Dec. 21, 2010 and submitted

Information
Nathan Binkert
gem5
Reviewers
Default
ali, gblack, nate, stever 
trace: reimplement the DTRACE function so it doesn't use a vector

One question I have about this stuff is if I should call everything trace, or debug?  This diff is somewhat confused about that (some things are trace and some things are debug) and I expect to fix it. We always called this stuff "trace flags" in the past, but we I would like to start using these flags for other things.  For example, turning on and off debugging breakpoints of different kinds.  Execution tracing is a totally different mechanism but does use trace flags.  My personal inclination is that trace flag is probably a bad name, but perhaps debug is a bad name too.  Just call it "flags"?  Or SimFlags?

   

  


 


 

  

  









  
 


  

 













 



 

  


 

  

   

   
Review request changed

   
Updated  (Dec. 21, 2010, 12:36 a.m.)

  

  







   



    

     
Description:

     
      
     
trace: reimplement the DTRACE function so it doesn't use a vector
   + 

   + 
One question I have about this stuff is if I should call everything trace, or debug?  This diff is somewhat confused about that (some things are trace and some things are debug) and I expect to fix it. We always called this stuff "trace flags" in the past, but we I would like to start using these flags for other things.  For example, turning on and off debugging breakpoints of different kinds.  Execution tracing is a totally different mechanism but does use trace flags.  My personal inclination is that trace flag is probably a bad name, but perhaps debug is a bad name too.  Just call it "flags"?  Or SimFlags?

     		
    



   




  

 


  

 



















 

  


 


  



   

   
Gabe Black

   
Posted  (Dec. 21, 2010, 6:05 a.m.)


  


  


  

   
I notice that most places include only one or two trace flag header files. There are a few places, though, where there are a bunch of trace flags included which are all related, something like "Ethernet,EthernetIntr". I'm assuming the base trace flag is a compound flag that includes the other ones. Would it make sense for the "trace/Enthernet.hh" header file to include the other ones and cut down on the header file sprawl?



  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    That is certainly possible, but I don't really think it is worth it.  There are only a few cases of this sort of thing happening (Ethernet is really the only one).  Also, in general, the different flags in a compound flags don't all show up in the same file, so using them would result in more #includes than necessary.  I could create src/dev/etherflags.hh  that is used by ns_gige.cc and sinic.cc, but those are the only two files that use several trace flags and at the same time use the same ones.  I won't strenuously object to your proposal, but I'm not highly motivated either.
    
    
    
   
    2. 

   


  





  

 


  

 



















 

  


 


  



   

   
Steve Reinhardt

   
Posted  (Dec. 21, 2010, 2:44 p.m.)


  


  


  

   
The name "trace flags" doesn't bother me, but "debug flags" is OK too.  I wouldn't want to be more generic than that though.



  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    I was thinking of adding a command line option for --debug-flags, but leaving the --trace-flags one there as well.  Sound reasonable?
    
    
    
   
    2. 


   
  2. 
    
    
     
    
      
     
    
     
    Is there a real need for backward compatibility?  Maybe we could have --trace-flags print a polite error message letting people know that the option has been renamed, so people's brains have time to retrain themselves, but if we really want to start using the new name then hanging on to the old option is only going to prolong the confusion IMO.  Others may differ...
    
    
    
   
    3. 


   
  3. 
    
    
     
    
      
     
    
     
    I guess not.  I thought you (and others) might complain.  I'll add an error message for --trace-flags, but I'll convert everything to debug flags.
    
    
    
   
    4. 

   


  




   







    

     
     

      

       
        

         
          src/base/trace.hh
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
If you're going to put the 'using namespace' here, why not delete 'Trace::' where it's currently used?





  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    I can't honestly say why I did that.  I'll get rid of it.  It could have had something to do with overloaded scope, but I'll at least try.  The "using namespace" stuff is there so the flags are in scope and you don't have to do Trace:: before each flag.  I could document that :)
    
    
    
   
    2. 

   


  


    


   



  

 


  

 

















 



 

  


 


  



   

   
Ali Saidi

   
Posted  (Jan. 4, 2011, 10:12 a.m.)


  


  


  

   



  

   

   
    
    
   


  




   







    

     
     

      

       
        

         
          src/base/debug.hh
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
This file needs doxygen comments. Minimally an @file comment, but preferably documenting each class as well.




  

   

   
    
    
   


  


    


    

     
     

      

       
        

         
          src/base/debug.hh
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
The things we do to not use varargs.... 




  

   

   
    
    
   


  


    


    

     
     

      

       
        

         
          src/base/debug.cc
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
These comments should really be doxygen comments

and there should be ones for the various functions below (e.g. findFlag)




  

   

   
    
    
   


  


    


    

     
     

      

       
        

         
          src/base/debug.cc
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
More comments please




  

   

   
    
    
   


  


    


    

     
     

      

       
        

         
          src/base/remote_gdb.cc
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
Mabye emitting the individual header files and the compound header file and the user can choose?




  

   

   
    
    
   


  


    


    

     
     

      

       
        

         
          src/base/trace.hh
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
Doxygen




  

   

   
    
    
   


  


    


    

     
     

      

       
        

         
          src/python/m5/debug.py
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
More comments please, at least what this file exists to do