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

[Alex Samorukov]

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

[Alex Samorukov]

Issue validated on CLANG4/Linux

[Alex Samorukov]

Related discussion ​https://reviews.freebsd.org/D9069?id=23667

[Christian Franke]

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)

[Alex Samorukov]

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.

[Christian Franke]

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

[Christian Franke]

• 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...

[Christian Franke]

• 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
  

[Alex Samorukov]

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

[Alex Samorukov]

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

[Alex Samorukov]

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.

[Christian Franke]

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).

[Christian Franke]

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

[Christian Franke]

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.

[Christian Franke]

Patch applied in r4555.