Page MenuHomePhabricator

Silence GCC's `-Wclass-memaccess` warnings
Needs ReviewPublic

Authored by chandlerc on Sep 16 2020, 3:33 AM.

Details

Reviewers
jhenderson
Summary

For folks hosting with GCC, this warning is especially annoying. It is
difficult to disable on the command line in all build systems because it
is a C++ specific warning. Disabling it produces still more warnings on
C code.

Suppressing the warning can be done easily by casting the pointer to
void *, but I'm also happy with any other suggested approach to
silence this warning in the source code.

Diff Detail

Event Timeline

chandlerc created this revision.Sep 16 2020, 3:33 AM
chandlerc requested review of this revision.Sep 16 2020, 3:33 AM

I've made two inline suggestions, but I'm not at all familiar with the other code, so can't further assist there.

llvm/lib/Target/X86/Disassembler/X86Disassembler.cpp
1719–1720

This should work, right?

llvm/tools/llvm-nm/llvm-nm.cpp
1638–1639

Can't you do this here (like in other places earlier in the code)? Not sure the memset is actually needed with that.

chandlerc added inline comments.Sep 16 2020, 3:53 AM
llvm/lib/Target/X86/Disassembler/X86Disassembler.cpp
1719–1720

Not necessarily?

They have at least potentially different semantics (for example, if the default constructor would leave some members uninitialized for some reason). I'm not super familiar with this code and didn't really want to actually change its meaning, just make the compiler warning stop. ;]

If you or another are confident that the code doesn't need the memset, I can switch. Stylistically, I'd suggest:

InternalInstruction Isns = {};

But that's a minor point.

jhenderson added inline comments.Sep 16 2020, 4:09 AM
llvm/lib/Target/X86/Disassembler/X86Disassembler.cpp
1719–1720

Looking at the definition of InternalInstruction, I see usage of ArrayRef, various enum and integer types, bools and a pointer. There is no user-specified default constructor, so this should result in everything being value initialized, which with the potential exception of the ArrayRef, is the same as setting everything to zero in this context. Setting ArrayRef to zero using memset looks like it's the same as the ArrayRef default constructor too, so it should be safe, I believe.

NB: I am by no means an expert in this code area, so I might be missing something.

I'm more confident in the llvm-nm case, and the same logic applies - there is no default constructor, and the non-trivial member types all zero-initialize themselves when default constructed.

rupprecht added inline comments.Sep 16 2020, 9:45 AM
llvm/lib/Target/X86/Disassembler/X86Disassembler.cpp
1719–1720

IMHO, we should respect the intent of that warning and not circumvent what constructors/assignment/copy constructors should be doing controlling. i.e. I prefer your suggestion of:

InternalInstruction Isns = {};

(and similar elsewhere)

As for preferring to keep this a NFC commit by silencing the warning with (void *) -- we have test coverage that should verify a semantic change is OK; we shouldn't treat these bits like haunted graveyards.