Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Weird (wrong) string code in papi.cpp #1274

Closed
eschnett opened this issue Sep 22, 2014 · 8 comments

Comments

Projects
None yet
3 participants
@eschnett
Copy link
Contributor

commented Sep 22, 2014

The file src/components/papi_counters/util/papi.cpp contains these lines, which look wrong:

            std::string note;
            if ((*it)->note && strlen((*it)->note) > 0)
            { // is this actually provided anywhere??
                note = "Note:\n";
                note += (*it)->note;
                note += "\n";
            }

This looks as if the string handling code was incompletely converted from C to C++; checking for NULL isn't commonly done for std::string.

@K-ballo

This comment has been minimized.

Copy link
Member

commented Sep 22, 2014

I can't find where (*it)->note comes from, but if it were std::string then the code would not compile.

@eschnett

This comment has been minimized.

@eschnett

This comment has been minimized.

@eschnett

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2014

I misread the code, note refers to two different entities here.

However, the code is still strange because clang reports this:

/project/eschnett/shelob/SIMFACTORY/hpx-master/src/hpx-master/src/components/papi_counters/util/papi.cpp:201:24: warning: address of array '(* it)->note' will always evaluate to 'true' [-Wpointer-bool-conversion]
            if ((*it)->note && strlen((*it)->note) > 0)
                ~~~~~~~^~~~ ~~
/project/eschnett/shelob/SIMFACTORY/hpx-master/src/hpx-master/src/components/papi_counters/util/papi.cpp:243:18: warning: address of array 'info.note' will always evaluate to 'true' [-Wpointer-bool-conversion]
        if (info.note && strlen(info.note) > 0)
            ~~~~~^~~~ ~~
@hkaiser

This comment has been minimized.

Copy link
Member

commented Sep 22, 2014

Please feel free to submit a pull request fixing those warnings.

@eschnett

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2014

Sorry, I don't understand the code well enough to do so. I don't know the type of the iterator generator_iterator_generator, nor could I find where the iterator's "meat" is declared -- there is one line typedef preset_enumerator<false> avail_preset_info_gen in papi.hpp, but that doesn't explain what kind of field note is. I'll leave this to someone who understands the code. I would have expected that this somehow points to PAPI events or PAPI messages, but I couldn't find the connection.

@hkaiser

This comment has been minimized.

Copy link
Member

commented Sep 23, 2014

Didn't you say (or clang did) that note is an array of characters, defined somewhere in the PAPI headers? So all what's needed is to remove the check whether info.note (or (*it)->note) is zero. But I can make that change.

@hkaiser

This comment has been minimized.

Copy link
Member

commented Sep 23, 2014

Here is the corresponding PAPI structure:

   typedef struct event_info {                                                          
      unsigned int event_code;             /**< preset (0x8xxxxxxx) or                  
                                                native (0x4xxxxxxx) event code */       
      char symbol[PAPI_HUGE_STR_LEN];      /**< name of the event */                    
      char short_descr[PAPI_MIN_STR_LEN];  /**< a short description suitable for        
                                                use as a label */                       
      char long_descr[PAPI_HUGE_STR_LEN];  /**< a longer description:                   
                                                typically a sentence for presets,       
                                                possibly a paragraph from vendor        
                                                docs for native events */               

      int component_index;           /**< component this event belongs to */            
      char units[PAPI_MIN_STR_LEN];  /**< units event is measured in */                 
      int location;                  /**< location event applies to */                  
      int data_type;                 /**< data type returned by PAPI */                 
      int value_type;                /**< sum or absolute */                            
      int timescope;                 /**< from start, etc. */                           
      int update_type;               /**< how event is updated */                       
      int update_freq;               /**< how frequently event is updated */            

     /* PRESET SPECIFIC FIELDS FOLLOW */                                                



      unsigned int count;                /**< number of terms (usually 1)               
                                              in the code and name fields               
                                              - presets: these are native events        
                                              - native: these are unused */             

      unsigned int event_type;           /**< event type or category                    
                                              for preset events only */                 

      char derived[PAPI_MIN_STR_LEN];    /**< name of the derived type                  
                                              - presets: usually NOT_DERIVED            
                                              - native: empty string */                 
      char postfix[PAPI_2MAX_STR_LEN];   /**< string containing postfix                 
                                              operations; only defined for              
                                              preset events of derived type             
                                              DERIVED_POSTFIX */                        

      unsigned int code[PAPI_MAX_INFO_TERMS]; /**< array of values that further         
                                              describe the event:                       
                                              - presets: native event_code values       
                                              - native:, register values(?) */          

      char name[PAPI_MAX_INFO_TERMS]         /**< names of code terms: */               
               [PAPI_2MAX_STR_LEN];          /**< - presets: native event names,        
                                                  - native: descriptive strings         
                                                  for each register value(?) */         

     char note[PAPI_HUGE_STR_LEN];          /**< an optional developer note             
                                                supplied with a preset event            
                                                to delineate platform specific          
                                                anomalies or restrictions */            

   } PAPI_event_info_t;                                                                 

So yes, clang is right (as usual). I'll remove the checks.

@hkaiser hkaiser closed this in b120ff2 Oct 2, 2014

hkaiser added a commit that referenced this issue Oct 4, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.