Mbed-os: Identically names structs in different files

[deece]

Description

When experimenting with LTO, the compiler has flagged a number of situations where the same struct occurs in multiple files, with different definitions.

The naming of these structs should be scoped to avoid collisions.

This may also have undesirable effects in builds where both definitions of the conflicting types are used.

See: https://travis-ci.org/ARMmbed/mbed-os/jobs/467813686

./features/storage/kvstore/filesystemstore/FileSystemStore.cpp:54:3: warning: type 'struct key_iterator_handle_t' violates the C++ One Definition Rule [-Wodr]
 } key_iterator_handle_t;
   ^
./features/storage/kvstore/securestore/SecureStore.cpp:73:3: note: a different type is defined in another translation unit
 } key_iterator_handle_t;
   ^
./features/storage/kvstore/filesystemstore/FileSystemStore.cpp:52:11: note: the first difference of corresponding definitions is field 'dir_handle'
     void *dir_handle;
           ^
./features/storage/kvstore/securestore/SecureStore.cpp:72:25: note: a field with different name is defined in another translation unit
     KVStore::iterator_t underlying_it;
                         ^
./features/storage/kvstore/filesystemstore/FileSystemStore.cpp:48:3: warning: type 'struct inc_set_handle_t' violates the C++ One Definition Rule [-Wodr]
 } inc_set_handle_t;
   ^
./features/storage/kvstore/securestore/SecureStore.cpp:68:3: note: a different type is defined in another translation unit
 } inc_set_handle_t;
   ^
./features/storage/kvstore/filesystemstore/FileSystemStore.cpp:44:11: note: the first difference of corresponding definitions is field 'key'
     char *key;
           ^
./features/storage/kvstore/securestore/SecureStore.cpp:61:23: note: a field with different name is defined in another translation unit
     record_metadata_t metadata;
                       ^
./features/storage/kvstore/tdbstore/TDBStore.cpp:60:3: warning: type 'struct master_record_data_t' violates the C++ One Definition Rule [-Wodr]
 } master_record_data_t;
   ^
./features/storage/nvstore/source/nvstore.cpp:66:3: note: a different type is defined in another translation unit
 } master_record_data_t;
   ^
./features/storage/kvstore/tdbstore/TDBStore.cpp:58:14: note: the first difference of corresponding definitions is field 'tdbstore_revision'
     uint16_t tdbstore_revision;
              ^
./features/storage/nvstore/source/nvstore.cpp:64:14: note: a field with different name is defined in another translation unit
     uint16_t max_keys;
              ^
./features/storage/kvstore/tdbstore/TDBStore.cpp:66:3: warning: type 'area_state_e' violates the C++ One Definition Rule [-Wodr]
 } area_state_e;
   ^
./features/storage/nvstore/source/nvstore.cpp:102:3: note: an enum with different value name is defined in another translation unit
 } area_state_e;
   ^
[ERROR] ./features/storage/kvstore/filesystemstore/FileSystemStore.cpp:54:3: warning: type 'struct key_iterator_handle_t' violates the C++ One Definition Rule [-Wodr]
 } key_iterator_handle_t;
   ^
./features/storage/kvstore/securestore/SecureStore.cpp:73:3: note: a different type is defined in another translation unit
 } key_iterator_handle_t;
   ^
./features/storage/kvstore/filesystemstore/FileSystemStore.cpp:52:11: note: the first difference of corresponding definitions is field 'dir_handle'
     void *dir_handle;
           ^
./features/storage/kvstore/securestore/SecureStore.cpp:72:25: note: a field with different name is defined in another translation unit
     KVStore::iterator_t underlying_it;
                         ^
./features/storage/kvstore/filesystemstore/FileSystemStore.cpp:48:3: warning: type 'struct inc_set_handle_t' violates the C++ One Definition Rule [-Wodr]
 } inc_set_handle_t;
   ^
./features/storage/kvstore/securestore/SecureStore.cpp:68:3: note: a different type is defined in another translation unit
 } inc_set_handle_t;
   ^
./features/storage/kvstore/filesystemstore/FileSystemStore.cpp:44:11: note: the first difference of corresponding definitions is field 'key'
     char *key;
           ^
./features/storage/kvstore/securestore/SecureStore.cpp:61:23: note: a field with different name is defined in another translation unit
     record_metadata_t metadata;
                       ^
./features/storage/kvstore/tdbstore/TDBStore.cpp:60:3: warning: type 'struct master_record_data_t' violates the C++ One Definition Rule [-Wodr]
 } master_record_data_t;
   ^
./features/storage/nvstore/source/nvstore.cpp:66:3: note: a different type is defined in another translation unit
 } master_record_data_t;
   ^
./features/storage/kvstore/tdbstore/TDBStore.cpp:58:14: note: the first difference of corresponding definitions is field 'tdbstore_revision'
     uint16_t tdbstore_revision;
              ^
./features/storage/nvstore/source/nvstore.cpp:64:14: note: a field with different name is defined in another translation unit
     uint16_t max_keys;
              ^
./features/storage/kvstore/tdbstore/TDBStore.cpp:66:3: warning: type 'area_state_e' violates the C++ One Definition Rule [-Wodr]
 } area_state_e;
   ^
./features/storage/nvstore/source/nvstore.cpp:102:3: note: an enum with different value name is defined in another translation unit
 } area_state_e;

To test: Rebase #9080 on top of the fixes and verify that there are no "one definition rule" warnings.

Issue request type

[ ] Question
[ ] Enhancement
[X] Bug

[ciarmcom]

Internal Jira reference: https://jira.arm.com/browse/MBOCUSTRIA-318

[deepikabhavnani]

CC @ARMmbed/mbed-os-storage

[kjbracey-arm]

This immediately smells like false positive to me. Why is the thing complaining about these in particular? They are already file-scoped - any sort of LTO should understand that, and be able to deal with it, surely?

Does it just complain about every same-named type in different C files?

Is the fix not just to disable the warning, if it copes correctly?

[kjbracey-arm]

Ah, okay, this is a C++ wrinkle that had escaped me: unlike C, top-level types aren't totally hidden within a translation unit - you can't use the same name for two different types in two different translation units.

Simplest fix here seems to be to just stick an anonymous namespace namespace { } around the type definitions.

Does the LTO cope if without complaint if you have matching structure names in two C files?

[deepikabhavnani]

Since these structures are common for all the modules, shouldn't they be part of common header file instead of duplicating in CPP/C files. https://github.com/ARMmbed/mbed-os/blob/0283bb84e469c75ebf8ded601312332ac099c633/docs/design-documents/features/storage/TDBStore/TDBStore_design.md#important-data-structures

Structures are different

[kjbracey-arm]

Similar structures doing similar jobs - hence the same names. But they're not actually the same definitions - hence the warnings.

[deepikabhavnani]

Yes noted that now

[davidsaada]

Will rename the structures to fix this.

[kjbracey-arm]

Just renaming is possibly not the best fix - the types are still public and could collide in future. If you really mean them to be local to that C++ file they should be wrapped in an anonymous namespace - the C++ equivalent of top-level static, except it works for both types and objects.

I can feel this is something I'm going to be nitting about in review from now on - I always tended to point out people failing to put static on their private objects, but now I'm aware of this type issue...

[deece]

Does the LTO cope if without complaint if you have matching structure names in two C files?

I believe so, as long as they are static.

[0xc0170]

Just renaming is possibly not the best fix - the types are still public and could collide in future. If you really mean them to be local to that C++ file they should be wrapped in an anonymous namespace - the C++ equivalent of top-level static, except it works for both types and objects.

I can feel this is something I'm going to be nitting about in review from now on - I always tended to point out people failing to put static on their private objects, but now I'm aware of this type issue...

cc @ARMmbed/mbed-os-maintainers