| Message ID | 156479006802.707590.7623788701230232646.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improvements for namespace creation/interrogation | expand |
Dan Williams <dan.j.williams@intel.com> writes: > gcc 9.1.1 emits a slew of warnings for many of the command field > accesses. I.e. warnings of the form: > > libndctl.c:2586:21: warning: taking address of packed member of ‘struct nd_cmd_get_config_data_hdr’ may result in an unaligned pointer value [-Waddress-of-packed-member] > 2586 | cmd->iter.offset = &cmd->get_data->in_offset; > | ^~~~~~~~~~~~~~~~~~~~~~~~~ > > Suppress these as fixing the warning would defeat the abstraction of being able > to have common code that operates on commands with common fields at different > offsets in the payload. As I understand it, taking a pointer to this potentially unaligned member can result in bus errors (or worse, accessing wrong data) on architectures that don't support unaligned accesses. I'd be a whole lot happier with this changelog if it mentioned that you had considered what the warning actually meant, and decided it didn't matter for the architectures you want to support. x86 is, of course, safe. I believe aarch64 is, as well. I didn't look into others. Cheers, Jeff > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > configure.ac | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/configure.ac b/configure.ac > index 4737cfff77f2..79f82977fa44 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -214,6 +214,7 @@ my_CFLAGS="\ > -Wmaybe-uninitialized \ > -Wdeclaration-after-statement \ > -Wunused-result \ > +-Wno-address-of-packed-member \ > -D_FORTIFY_SOURCE=2 \ > -O2 > " > > _______________________________________________ > Linux-nvdimm mailing list > Linux-nvdimm@lists.01.org > https://lists.01.org/mailman/listinfo/linux-nvdimm
On Mon, Aug 5, 2019 at 10:06 AM Jeff Moyer <jmoyer@redhat.com> wrote: > > Dan Williams <dan.j.williams@intel.com> writes: > > > gcc 9.1.1 emits a slew of warnings for many of the command field > > accesses. I.e. warnings of the form: > > > > libndctl.c:2586:21: warning: taking address of packed member of ‘struct nd_cmd_get_config_data_hdr’ may result in an unaligned pointer value [-Waddress-of-packed-member] > > 2586 | cmd->iter.offset = &cmd->get_data->in_offset; > > | ^~~~~~~~~~~~~~~~~~~~~~~~~ > > > > Suppress these as fixing the warning would defeat the abstraction of being able > > to have common code that operates on commands with common fields at different > > offsets in the payload. > > As I understand it, taking a pointer to this potentially unaligned > member can result in bus errors (or worse, accessing wrong data) on > architectures that don't support unaligned accesses. I'd be a whole lot > happier with this changelog if it mentioned that you had considered what > the warning actually meant, and decided it didn't matter for the > architectures you want to support. True, it was a fleeting thought, but not something I considered fleshing out... I'll send a revision. > > x86 is, of course, safe. I believe aarch64 is, as well. I didn't look > into others. > Keep in mind that this code is for interfacing with the ACPI DSM path. If an unaligned-incapable architecture defined an NVDIMM command set it is highly unlikely it would be ACPI based, or pick these problematic command formats. I can add these notes to the changelog, but the unfortunate definition of these payloads that require __packed is something I hope other architectures avoid.
Dan Williams <dan.j.williams@intel.com> writes: > On Mon, Aug 5, 2019 at 10:06 AM Jeff Moyer <jmoyer@redhat.com> wrote: >> >> Dan Williams <dan.j.williams@intel.com> writes: >> >> > gcc 9.1.1 emits a slew of warnings for many of the command field >> > accesses. I.e. warnings of the form: >> > >> > libndctl.c:2586:21: warning: taking address of packed member of ‘struct nd_cmd_get_config_data_hdr’ may result in an unaligned pointer value [-Waddress-of-packed-member] >> > 2586 | cmd->iter.offset = &cmd->get_data->in_offset; >> > | ^~~~~~~~~~~~~~~~~~~~~~~~~ >> > >> > Suppress these as fixing the warning would defeat the abstraction of being able >> > to have common code that operates on commands with common fields at different >> > offsets in the payload. >> >> As I understand it, taking a pointer to this potentially unaligned >> member can result in bus errors (or worse, accessing wrong data) on >> architectures that don't support unaligned accesses. I'd be a whole lot >> happier with this changelog if it mentioned that you had considered what >> the warning actually meant, and decided it didn't matter for the >> architectures you want to support. > > True, it was a fleeting thought, but not something I considered > fleshing out... I'll send a revision. Thanks. >> x86 is, of course, safe. I believe aarch64 is, as well. I didn't look >> into others. >> > Keep in mind that this code is for interfacing with the ACPI DSM path. > If an unaligned-incapable architecture defined an NVDIMM command set > it is highly unlikely it would be ACPI based, or pick these > problematic command formats. I can add these notes to the changelog, > but the unfortunate definition of these payloads that require __packed > is something I hope other architectures avoid. We can hope... :) Anyway, thanks for the additional context. Cheers, Jeff
On Mon, Aug 5, 2019 at 10:34 AM Dan Williams <dan.j.williams@intel.com> wrote: > > On Mon, Aug 5, 2019 at 10:06 AM Jeff Moyer <jmoyer@redhat.com> wrote: > > > > Dan Williams <dan.j.williams@intel.com> writes: > > > > > gcc 9.1.1 emits a slew of warnings for many of the command field > > > accesses. I.e. warnings of the form: > > > > > > libndctl.c:2586:21: warning: taking address of packed member of ‘struct nd_cmd_get_config_data_hdr’ may result in an unaligned pointer value [-Waddress-of-packed-member] > > > 2586 | cmd->iter.offset = &cmd->get_data->in_offset; > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > Suppress these as fixing the warning would defeat the abstraction of being able > > > to have common code that operates on commands with common fields at different > > > offsets in the payload. > > > > As I understand it, taking a pointer to this potentially unaligned > > member can result in bus errors (or worse, accessing wrong data) on > > architectures that don't support unaligned accesses. I'd be a whole lot > > happier with this changelog if it mentioned that you had considered what > > the warning actually meant, and decided it didn't matter for the > > architectures you want to support. > > True, it was a fleeting thought, but not something I considered > fleshing out... I'll send a revision. > > > > > x86 is, of course, safe. I believe aarch64 is, as well. I didn't look > > into others. > > > > Keep in mind that this code is for interfacing with the ACPI DSM path. > If an unaligned-incapable architecture defined an NVDIMM command set > it is highly unlikely it would be ACPI based, or pick these > problematic command formats. I can add these notes to the changelog, > but the unfortunate definition of these payloads that require __packed > is something I hope other architectures avoid. ...and it turns out this is wrong because we have the default ioctls that also use these packed structures ND_CMD_ARS_CAP = 1, ND_CMD_ARS_START = 2, ND_CMD_ARS_STATUS = 3, ND_CMD_CLEAR_ERROR = 4, ND_CMD_SMART = 1, ND_CMD_SMART_THRESHOLD = 2, ND_CMD_DIMM_FLAGS = 3, ND_CMD_GET_CONFIG_SIZE = 4, ND_CMD_GET_CONFIG_DATA = 5, ND_CMD_SET_CONFIG_DATA = 6, ND_CMD_VENDOR = 9, I'll take a look at reworking this.
diff --git a/configure.ac b/configure.ac index 4737cfff77f2..79f82977fa44 100644 --- a/configure.ac +++ b/configure.ac @@ -214,6 +214,7 @@ my_CFLAGS="\ -Wmaybe-uninitialized \ -Wdeclaration-after-statement \ -Wunused-result \ +-Wno-address-of-packed-member \ -D_FORTIFY_SOURCE=2 \ -O2 "
gcc 9.1.1 emits a slew of warnings for many of the command field accesses. I.e. warnings of the form: libndctl.c:2586:21: warning: taking address of packed member of ‘struct nd_cmd_get_config_data_hdr’ may result in an unaligned pointer value [-Waddress-of-packed-member] 2586 | cmd->iter.offset = &cmd->get_data->in_offset; | ^~~~~~~~~~~~~~~~~~~~~~~~~ Suppress these as fixing the warning would defeat the abstraction of being able to have common code that operates on commands with common fields at different offsets in the payload. Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- configure.ac | 1 + 1 file changed, 1 insertion(+)