Opened 6 years ago

Closed 6 years ago

#915 closed defect (fixed)

Clang generates warnings on compilation "taking address of packed member" on the atacmds.cpp

Reported by: Alex Samorukov Owned by: Christian Franke
Priority: minor Milestone: Release 6.6
Component: all Version: 6.5
Keywords: Cc:

Description

atacmds.cpp:1004:12: warning: taking address of packed member 'extend_test_completion_time_w' of class or structure 'ata_smart_values' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&data->extend_test_completion_time_w);
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
atacmds.cpp:1091:12: warning: taking address of packed member 'log_desc_index' of class or structure 'ata_smart_extselftestlog' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&log->log_desc_index);
           ^~~~~~~~~~~~~~~~~~~
atacmds.cpp:1198:12: warning: taking address of packed member 'logversion' of class or structure 'ata_smart_log_directory' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&data->logversion);
           ^~~~~~~~~~~~~~~~
atacmds.cpp:1521:12: warning: taking address of packed member 'device_error_count' of class or structure 'ata_smart_exterrlog' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&log->device_error_count);
           ^~~~~~~~~~~~~~~~~~~~~~~
atacmds.cpp:1522:12: warning: taking address of packed member 'error_log_index' of class or structure 'ata_smart_exterrlog' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&log->error_log_index);
           ^~~~~~~~~~~~~~~~~~~~
atacmds.cpp:2260:12: warning: taking address of packed member 'format_version' of class or structure 'ata_sct_status_response' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&sts->format_version);
           ^~~~~~~~~~~~~~~~~~~
atacmds.cpp:2261:12: warning: taking address of packed member 'sct_version' of class or structure 'ata_sct_status_response' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&sts->sct_version);
           ^~~~~~~~~~~~~~~~
atacmds.cpp:2262:12: warning: taking address of packed member 'sct_spec' of class or structure 'ata_sct_status_response' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&sts->sct_spec);
           ^~~~~~~~~~~~~
atacmds.cpp:2263:12: warning: taking address of packed member 'ext_status_code' of class or structure 'ata_sct_status_response' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&sts->ext_status_code);
           ^~~~~~~~~~~~~~~~~~~~
atacmds.cpp:2264:12: warning: taking address of packed member 'action_code' of class or structure 'ata_sct_status_response' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&sts->action_code);
           ^~~~~~~~~~~~~~~~
atacmds.cpp:2265:12: warning: taking address of packed member 'function_code' of class or structure 'ata_sct_status_response' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&sts->function_code);
           ^~~~~~~~~~~~~~~~~~
atacmds.cpp:2266:12: warning: taking address of packed member 'over_limit_count' of class or structure 'ata_sct_status_response' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&sts->over_limit_count);
           ^~~~~~~~~~~~~~~~~~~~~
atacmds.cpp:2267:12: warning: taking address of packed member 'under_limit_count' of class or structure 'ata_sct_status_response' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&sts->under_limit_count);
           ^~~~~~~~~~~~~~~~~~~~~~
atacmds.cpp:2268:12: warning: taking address of packed member 'smart_status' of class or structure 'ata_sct_status_response' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&sts->smart_status);
           ^~~~~~~~~~~~~~~~~
atacmds.cpp:2269:12: warning: taking address of packed member 'min_erc_time' of class or structure 'ata_sct_status_response' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&sts->min_erc_time);
           ^~~~~~~~~~~~~~~~~
atacmds.cpp:2302:12: warning: taking address of packed member 'action_code' of class or structure 'ata_sct_data_table_command' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&cmd.action_code);
           ^~~~~~~~~~~~~~~
atacmds.cpp:2303:12: warning: taking address of packed member 'function_code' of class or structure 'ata_sct_data_table_command' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&cmd.function_code);
           ^~~~~~~~~~~~~~~~~
atacmds.cpp:2304:12: warning: taking address of packed member 'table_id' of class or structure 'ata_sct_data_table_command' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&cmd.table_id);
           ^~~~~~~~~~~~
atacmds.cpp:2332:12: warning: taking address of packed member 'format_version' of class or structure 'ata_sct_temperature_history_table' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&tmh->format_version);
           ^~~~~~~~~~~~~~~~~~~
atacmds.cpp:2333:12: warning: taking address of packed member 'sampling_period' of class or structure 'ata_sct_temperature_history_table' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&tmh->sampling_period);
           ^~~~~~~~~~~~~~~~~~~~
atacmds.cpp:2334:12: warning: taking address of packed member 'interval' of class or structure 'ata_sct_temperature_history_table' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&tmh->interval);
           ^~~~~~~~~~~~~
atacmds.cpp:2335:12: warning: taking address of packed member 'cb_index' of class or structure 'ata_sct_temperature_history_table' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&tmh->cb_index);
           ^~~~~~~~~~~~~
atacmds.cpp:2336:12: warning: taking address of packed member 'cb_size' of class or structure 'ata_sct_temperature_history_table' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&tmh->cb_size);
           ^~~~~~~~~~~~
atacmds.cpp:2369:12: warning: taking address of packed member 'action_code' of class or structure 'ata_sct_feature_control_command' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&cmd.action_code);
           ^~~~~~~~~~~~~~~
atacmds.cpp:2370:12: warning: taking address of packed member 'function_code' of class or structure 'ata_sct_feature_control_command' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&cmd.function_code);
           ^~~~~~~~~~~~~~~~~
atacmds.cpp:2371:12: warning: taking address of packed member 'feature_code' of class or structure 'ata_sct_feature_control_command' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&cmd.feature_code);
           ^~~~~~~~~~~~~~~~
atacmds.cpp:2372:12: warning: taking address of packed member 'state' of class or structure 'ata_sct_feature_control_command' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&cmd.state);
           ^~~~~~~~~
atacmds.cpp:2373:12: warning: taking address of packed member 'option_flags' of class or structure 'ata_sct_feature_control_command' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&cmd.option_flags);
           ^~~~~~~~~~~~~~~~
atacmds.cpp:2449:12: warning: taking address of packed member 'action_code' of class or structure 'ata_sct_feature_control_command' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&cmd.action_code);
           ^~~~~~~~~~~~~~~
atacmds.cpp:2450:12: warning: taking address of packed member 'function_code' of class or structure 'ata_sct_feature_control_command' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&cmd.function_code);
           ^~~~~~~~~~~~~~~~~
atacmds.cpp:2451:12: warning: taking address of packed member 'feature_code' of class or structure 'ata_sct_feature_control_command' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&cmd.feature_code);
           ^~~~~~~~~~~~~~~~
atacmds.cpp:2452:12: warning: taking address of packed member 'state' of class or structure 'ata_sct_feature_control_command' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&cmd.state);
           ^~~~~~~~~
atacmds.cpp:2453:12: warning: taking address of packed member 'option_flags' of class or structure 'ata_sct_feature_control_command' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&cmd.option_flags);
           ^~~~~~~~~~~~~~~~
atacmds.cpp:2501:12: warning: taking address of packed member 'action_code' of class or structure 'ata_sct_error_recovery_control_command' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&cmd.action_code);
           ^~~~~~~~~~~~~~~
atacmds.cpp:2502:12: warning: taking address of packed member 'function_code' of class or structure 'ata_sct_error_recovery_control_command' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&cmd.function_code);
           ^~~~~~~~~~~~~~~~~
atacmds.cpp:2503:12: warning: taking address of packed member 'selection_code' of class or structure 'ata_sct_error_recovery_control_command' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&cmd.selection_code);
           ^~~~~~~~~~~~~~~~~~
atacmds.cpp:2504:12: warning: taking address of packed member 'time_limit' of class or structure 'ata_sct_error_recovery_control_command' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
    swapx(&cmd.time_limit);
           ^~~~~~~~~~~~~~
37 warnings generated.

Clang version:

clang -v
Apple LLVM version 9.0.0 (clang-900.0.37)
Target: x86_64-apple-darwin16.7.0
Thread model: posix
InstalledDir: /Library/Developer/CommandLineTools/usr/bin

Attachments (1)

ticket-915.patch (5.4 KB ) - added by Christian Franke 6 years ago.
Proposed patch for atacmds.cpp: replace swapx(&x) by SWAPV(x)

Download all attachments as: .zip

Change History (14)

comment:1 by Alex Samorukov, 6 years ago

Issue validated on CLANG4/Linux

comment:3 by Christian Franke, 6 years ago

Also validated with Cygwin Clang 4.0.

All warnings could be safely ignored as the pointers are later reinterpreted as char *.

To avoid regressions due to unnecessary code changes and avoid complaints about the warnings, I would suggest to add this to atacmds.cpp for release 6.6:

#if __clang_major__ >= 4
#pragma clang diagnostic ignored "-Waddress-of-packed-member"
#endif

For the future, we have these options:

  • Keep the above, or
  • carefully replace swapx() by old swap[248]() calls on packed members (after replacing char * by void * to avoid ugly casts in the calls), or
  • rework structs and member access such that packed structs are no longer needed (like in NVMe structs where swapx() is also used)
Last edited 6 years ago by Christian Franke (previous) (diff)

comment:4 by Alex Samorukov, 6 years ago

I checked that warnings are about structures which are already aligned, e.g. 512 bytes length. I think we can safely remove pack pragma for them. This would make compiler silent and also we still have ASSERT with sizeof, just in case.

I can make a patch for review to disable packing of such structures.

Last edited 6 years ago by Alex Samorukov (previous) (diff)

comment:5 by Christian Franke, 6 years ago

OK, but be careful as offsetof(some_struct, some_member) may change without affecting sizeof(some_struct).

comment:6 by Christian Franke, 6 years ago

  • or, avoid pointers at all but still benefit from C++ overloading (drawback: #define required):
    unsigned short swapval(unsigned short v);
    unsigned int   swapval(unsigned int v);
    uint64_t       swapval(uint64_t v);
    #define swapx(x) ((x) = swapval(x))
    

BTW: Compilers 'opinions' are really different here. If C++ references are used instead of pointers, clang warning disappears but gcc refuses compilation:

struct S {
 char c; unsigned int i;
} __attribute((__packed__));

void swapx(unsigned int * p);
void swapr(unsigned int & r);

void f(S * s)
{
   // clang++: warning: taking address of packed member 'i' ...
   // g++ -Wall -W: silent
   swapx(&s->i);
   // clang++ -Wall -W: silent
   // g++: error: cannot bind packed field ‘s->S::i’ to ‘unsigned int&’
   swapr(s->i);
}

BTW2: Clang warning does not appear if #pragma packed is used:

struct S1 { char c; unsigned int i; } __attribute((__packed__));

#pragma pack(1)
struct S2 { char c; unsigned int i; };
#pragma pack()

// -std=c++11
static_assert(sizeof(S1) == 5 && sizeof(S2) == 5, "not packed");

void swapx(unsigned int * p);

void f(S1 * s1, S2 * s2)
{
  swapx(&s1->i); // clang++: taking address of packed member 'i' ...
  swapx(&s2->i); // clang++: silent
}

Hmmm...

comment:7 by Christian Franke, 6 years ago

  • or, avoid compilation of later-optimized-away swapx() calls on LE platforms (and learn which BE platforms are still relevant for smartmontools):
    - if (isbigendian()) {
    + #ifdef WORDS_BIGENDIAN
    + {
        swapx(&...);
        ...
      }
    + #endif // WORDS_BIGENDIAN
    

comment:8 by Alex Samorukov, 6 years ago

I was also overoptimistic about aligment - disabling it causing ata_smart_values to be 514 bytes.

comment:9 by Alex Samorukov, 6 years ago

I think clang silently ignores #pragma packed, thats why there are no warnings

comment:10 by Alex Samorukov, 6 years ago

I think BE is still more or less widespread on SPARC hardware and some PowerPC based appliances. I had a BE NAS some years ago, so i think there are still some of them in the wild :)

P.S. still have 2 routers with USB port running on PPC/OpenWRT. Need to check if it is BE, but most likely yes.

Last edited 6 years ago by Alex Samorukov (previous) (diff)

comment:11 by Christian Franke, 6 years ago

Then using #ifdef WORD_BIGENDIAN is not an option as syntax of BE code would then no longer be checked in the 'mainstream' (all LE) builds.

For the short term (Release 6.6), I would suggest to simply disable the warning in the affected module (see above). Changes to BE specific code should only be done if someone is able to test this (see also #117).

For the longer term, I have already planned to move all this struct member swapping code to separate struct_foo_to_cpu() functions (useful in a later API), fix the inconsistency in ATA LE string handling and add some runtime tests (see check_config()). Any use of addresses of packed members could be avoided then (e.g. with p->x = swap(p->x)).

PS:
Clang honors #pragma packed, otherwise the static_assert in the above code would fail.

The __attribute__((packed)) was added long ago (r2014) because some older GCC versions ignored #pragma pack. Vice versa the pragma is still necessary even for GCC due to a long standing bug in MinGW GCC (still present in 6.3.0).

by Christian Franke, 6 years ago

Attachment: ticket-915.patch added

Proposed patch for atacmds.cpp: replace swapx(&x) by SWAPV(x)

comment:12 by Christian Franke, 6 years ago

Milestone: undecidedRelease 6.6
Owner: set to Christian Franke
Status: newaccepted

Above patch prevents unaligned pointers and silences the warnings. Regression risk is IMO low and extra code uglyness is moderate. Suggest to include this into Release 6.6. More rework will be done later.

comment:13 by Christian Franke, 6 years ago

Resolution: fixed
Status: acceptedclosed

Patch applied in r4555.

Note: See TracTickets for help on using tickets.