On Fri, Oct 27, 2017 at 08:48:07AM -0500, Eric Sandeen wrote:
> On 10/26/17 5:15 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >
> > Fix all the things ubsan warned about.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > ---
> > include/bitops.h | 8 ++++----
> > include/command.h | 2 +-
> > include/xfs_arch.h | 2 +-
> > 3 files changed, 6 insertions(+), 6 deletions(-)
> >
> >
> > diff --git a/include/bitops.h b/include/bitops.h
> > index 44599a7..53a5aa0 100644
> > --- a/include/bitops.h
> > +++ b/include/bitops.h
> > @@ -13,19 +13,19 @@ static inline int fls(int x)
> > if (!x)
> > return 0;
> > if (!(x & 0xffff0000u)) {
> > - x <<= 16;
> > + x = (x & 0xFFFFU) << 16;
>
> xfsprogs almost exclusively uses lowercase hex - as it does in the line
> above your new code. ;) I can change it on the way in unless you object.
Ok, sounds good to me.
> > r -= 16;
> > }
> > if (!(x & 0xff000000u)) {
> > - x <<= 8;
> > + x = (x & 0xFFFFFFU) << 8;
> > r -= 8;
> > }
> > if (!(x & 0xf0000000u)) {
> > - x <<= 4;
> > + x = (x & 0xFFFFFFFU) << 4;
> > r -= 4;
> > }
> > if (!(x & 0xc0000000u)) {
> > - x <<= 2;
> > + x = (x & 0x3FFFFFFFU) << 2;
> > r -= 2;
> > }
> > if (!(x & 0x80000000u)) {
> > diff --git a/include/command.h b/include/command.h
> > index fb3f5c7..59eab96 100644
> > --- a/include/command.h
> > +++ b/include/command.h
> > @@ -25,7 +25,7 @@
> > * not iterate the command args function callout and so can be used
> > * for functions like "help" that should only ever be run once.
> > */
> > -#define CMD_FLAG_ONESHOT (1<<31)
> > +#define CMD_FLAG_ONESHOT (1U<<31)
> > #define CMD_FLAG_FOREIGN_OK (1<<30) /* command not restricted to XFS */
> > #define CMD_FLAG_LIBRARY (1<<29) /* command provided by libxcmd */
>
> What complained about this? cmd flags is a signed int, right, so why is
> this a problem? (And why are we only using the top 3 bits? And let's just do 1U
> on every flag if it's really needed, but why is it needed? I must be missing
> something.)
I think the complaint is that "1" is signed int, and ubsan doesn't like
us shifting a value bit into the sign bit.
But yeah, I suppose if we 'U'ize one of them we could do it for all.
(Though really, why not start with the lower bits?)
--D
> > diff --git a/include/xfs_arch.h b/include/xfs_arch.h
> > index 186cadb..34fcd4d 100644
> > --- a/include/xfs_arch.h
> > +++ b/include/xfs_arch.h
> > @@ -253,7 +253,7 @@ static inline uint16_t get_unaligned_be16(void *p)
> > static inline uint32_t get_unaligned_be32(void *p)
> > {
> > uint8_t *__p = p;
> > - return __p[0] << 24 | __p[1] << 16 | __p[2] << 8 | __p[3];
> > + return (uint32_t)__p[0] << 24 | __p[1] << 16 | __p[2] << 8 | __p[3];
> > }
> >
> > static inline uint64_t get_unaligned_be64(void *p)
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
