Leif, Why does the current definition strictly require alignment?
I know we have used this for a long time with many compilers passing in unaligned pointers to UINT16, UINT32, UINT64 values without any compiler warnings/errors. Also, the patch contains locals that are initialized in the declaration of the local. We prefer locals to be initialized in the body of the function. Thanks, Mike > -----Original Message----- > From: Leif Lindholm [mailto:[email protected]] > Sent: Thursday, May 12, 2016 3:22 PM > To: [email protected] > Cc: Kinney, Michael D <[email protected]>; Gao, Liming > <[email protected]>; Ard Biesheuvel <[email protected]> > Subject: [RFC] MdePkg: BaseLib: don't use aligned pointers for unaligned > accessors > > The ReadUnaligned##/WriteUnaligned## functions provide a portable way of > accessing potentially unaligned locations, but the prototypes in > BaseLib.h all specify strictly aligned pointers. > > Change the prototypes as well as the implementations across Ia32/X64, > ARM/AArch64 and IPF to use VOID * pointers instead. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Leif Lindholm <[email protected]> > --- > > Built and tested for ArmVirtPkg for AArch64/ARM DEBUG/RELEASE. > Build tested only for OvmfPkg for Ia32/X64 DEBUG/RELEASE. > Not tested at all for IPF. > > I am not entirely convinced by the BitFieldWrite32 logic in > WriteUnaligned24, but this patch is not the place to fix this. > > I am also not convinced there is much point in keeping separate > implementations for IPF and ARM* - the optimization issues mentioned > in the git log could even have been affected by the incorrect > pointer types. > > This is the first patch triggered by my CLANG -Weverything exercises > earlier this year. More to come. > > MdePkg/Include/Library/BaseLib.h | 16 ++++++------ > MdePkg/Library/BaseLib/Arm/Unaligned.c | 16 ++++++------ > MdePkg/Library/BaseLib/Ipf/Unaligned.c | 16 ++++++------ > MdePkg/Library/BaseLib/Unaligned.c | 48 > ++++++++++++++++++++++------------ > 4 files changed, 56 insertions(+), 40 deletions(-) > > diff --git a/MdePkg/Include/Library/BaseLib.h > b/MdePkg/Include/Library/BaseLib.h > index c41fa78..133656a 100644 > --- a/MdePkg/Include/Library/BaseLib.h > +++ b/MdePkg/Include/Library/BaseLib.h > @@ -2549,7 +2549,7 @@ DivS64x64Remainder ( > UINT16 > EFIAPI > ReadUnaligned16 ( > - IN CONST UINT16 *Buffer > + IN CONST VOID *Buffer > ); > > > @@ -2571,7 +2571,7 @@ ReadUnaligned16 ( > UINT16 > EFIAPI > WriteUnaligned16 ( > - OUT UINT16 *Buffer, > + OUT VOID *Buffer, > IN UINT16 Value > ); > > @@ -2592,7 +2592,7 @@ WriteUnaligned16 ( > UINT32 > EFIAPI > ReadUnaligned24 ( > - IN CONST UINT32 *Buffer > + IN CONST VOID *Buffer > ); > > > @@ -2614,7 +2614,7 @@ ReadUnaligned24 ( > UINT32 > EFIAPI > WriteUnaligned24 ( > - OUT UINT32 *Buffer, > + OUT VOID *Buffer, > IN UINT32 Value > ); > > @@ -2635,7 +2635,7 @@ WriteUnaligned24 ( > UINT32 > EFIAPI > ReadUnaligned32 ( > - IN CONST UINT32 *Buffer > + IN CONST VOID *Buffer > ); > > > @@ -2657,7 +2657,7 @@ ReadUnaligned32 ( > UINT32 > EFIAPI > WriteUnaligned32 ( > - OUT UINT32 *Buffer, > + OUT VOID *Buffer, > IN UINT32 Value > ); > > @@ -2678,7 +2678,7 @@ WriteUnaligned32 ( > UINT64 > EFIAPI > ReadUnaligned64 ( > - IN CONST UINT64 *Buffer > + IN CONST VOID *Buffer > ); > > > @@ -2700,7 +2700,7 @@ ReadUnaligned64 ( > UINT64 > EFIAPI > WriteUnaligned64 ( > - OUT UINT64 *Buffer, > + OUT VOID *Buffer, > IN UINT64 Value > ); > > diff --git a/MdePkg/Library/BaseLib/Arm/Unaligned.c > b/MdePkg/Library/BaseLib/Arm/Unaligned.c > index 34f1732..ef9efbc 100644 > --- a/MdePkg/Library/BaseLib/Arm/Unaligned.c > +++ b/MdePkg/Library/BaseLib/Arm/Unaligned.c > @@ -33,7 +33,7 @@ > UINT16 > EFIAPI > ReadUnaligned16 ( > - IN CONST UINT16 *Buffer > + IN CONST VOID *Buffer > ) > { > volatile UINT8 LowerByte; > @@ -65,7 +65,7 @@ ReadUnaligned16 ( > UINT16 > EFIAPI > WriteUnaligned16 ( > - OUT UINT16 *Buffer, > + OUT VOID *Buffer, > IN UINT16 Value > ) > { > @@ -93,7 +93,7 @@ WriteUnaligned16 ( > UINT32 > EFIAPI > ReadUnaligned24 ( > - IN CONST UINT32 *Buffer > + IN CONST VOID *Buffer > ) > { > ASSERT (Buffer != NULL); > @@ -122,7 +122,7 @@ ReadUnaligned24 ( > UINT32 > EFIAPI > WriteUnaligned24 ( > - OUT UINT32 *Buffer, > + OUT VOID *Buffer, > IN UINT32 Value > ) > { > @@ -149,7 +149,7 @@ WriteUnaligned24 ( > UINT32 > EFIAPI > ReadUnaligned32 ( > - IN CONST UINT32 *Buffer > + IN CONST VOID *Buffer > ) > { > UINT16 LowerBytes; > @@ -181,7 +181,7 @@ ReadUnaligned32 ( > UINT32 > EFIAPI > WriteUnaligned32 ( > - OUT UINT32 *Buffer, > + OUT VOID *Buffer, > IN UINT32 Value > ) > { > @@ -208,7 +208,7 @@ WriteUnaligned32 ( > UINT64 > EFIAPI > ReadUnaligned64 ( > - IN CONST UINT64 *Buffer > + IN CONST VOID *Buffer > ) > { > UINT32 LowerBytes; > @@ -240,7 +240,7 @@ ReadUnaligned64 ( > UINT64 > EFIAPI > WriteUnaligned64 ( > - OUT UINT64 *Buffer, > + OUT VOID *Buffer, > IN UINT64 Value > ) > { > diff --git a/MdePkg/Library/BaseLib/Ipf/Unaligned.c > b/MdePkg/Library/BaseLib/Ipf/Unaligned.c > index 7d0d8dd..5ba8029 100644 > --- a/MdePkg/Library/BaseLib/Ipf/Unaligned.c > +++ b/MdePkg/Library/BaseLib/Ipf/Unaligned.c > @@ -30,7 +30,7 @@ > UINT16 > EFIAPI > ReadUnaligned16 ( > - IN CONST UINT16 *Buffer > + IN CONST VOID *Buffer > ) > { > ASSERT (Buffer != NULL); > @@ -56,7 +56,7 @@ ReadUnaligned16 ( > UINT16 > EFIAPI > WriteUnaligned16 ( > - OUT UINT16 *Buffer, > + OUT VOID *Buffer, > IN UINT16 Value > ) > { > @@ -84,7 +84,7 @@ WriteUnaligned16 ( > UINT32 > EFIAPI > ReadUnaligned24 ( > - IN CONST UINT32 *Buffer > + IN CONST VOID *Buffer > ) > { > ASSERT (Buffer != NULL); > @@ -113,7 +113,7 @@ ReadUnaligned24 ( > UINT32 > EFIAPI > WriteUnaligned24 ( > - OUT UINT32 *Buffer, > + OUT VOID *Buffer, > IN UINT32 Value > ) > { > @@ -140,7 +140,7 @@ WriteUnaligned24 ( > UINT32 > EFIAPI > ReadUnaligned32 ( > - IN CONST UINT32 *Buffer > + IN CONST VOID *Buffer > ) > { > UINT16 LowerBytes; > @@ -172,7 +172,7 @@ ReadUnaligned32 ( > UINT32 > EFIAPI > WriteUnaligned32 ( > - OUT UINT32 *Buffer, > + OUT VOID *Buffer, > IN UINT32 Value > ) > { > @@ -199,7 +199,7 @@ WriteUnaligned32 ( > UINT64 > EFIAPI > ReadUnaligned64 ( > - IN CONST UINT64 *Buffer > + IN CONST VOID *Buffer > ) > { > UINT32 LowerBytes; > @@ -231,7 +231,7 @@ ReadUnaligned64 ( > UINT64 > EFIAPI > WriteUnaligned64 ( > - OUT UINT64 *Buffer, > + OUT VOID *Buffer, > IN UINT64 Value > ) > { > diff --git a/MdePkg/Library/BaseLib/Unaligned.c > b/MdePkg/Library/BaseLib/Unaligned.c > index 68dafa6..ed58861 100644 > --- a/MdePkg/Library/BaseLib/Unaligned.c > +++ b/MdePkg/Library/BaseLib/Unaligned.c > @@ -32,12 +32,14 @@ > UINT16 > EFIAPI > ReadUnaligned16 ( > - IN CONST UINT16 *Buffer > + IN CONST VOID *Buffer > ) > { > + CONST UINT16 *Input = Buffer; > + > ASSERT (Buffer != NULL); > > - return *Buffer; > + return *Input; > } > > /** > @@ -58,13 +60,15 @@ ReadUnaligned16 ( > UINT16 > EFIAPI > WriteUnaligned16 ( > - OUT UINT16 *Buffer, > + OUT VOID *Buffer, > IN UINT16 Value > ) > { > + UINT16 *Output = Buffer; > + > ASSERT (Buffer != NULL); > > - return *Buffer = Value; > + return *Output = Value; > } > > /** > @@ -83,12 +87,14 @@ WriteUnaligned16 ( > UINT32 > EFIAPI > ReadUnaligned24 ( > - IN CONST UINT32 *Buffer > + IN CONST VOID *Buffer > ) > { > + CONST UINT32 *Input = Buffer; > + > ASSERT (Buffer != NULL); > > - return *Buffer & 0xffffff; > + return *Input & 0xffffff; > } > > /** > @@ -109,13 +115,15 @@ ReadUnaligned24 ( > UINT32 > EFIAPI > WriteUnaligned24 ( > - OUT UINT32 *Buffer, > + OUT VOID *Buffer, > IN UINT32 Value > ) > { > + UINT32 *Output = Buffer; > + > ASSERT (Buffer != NULL); > > - *Buffer = BitFieldWrite32 (*Buffer, 0, 23, Value); > + *Output = BitFieldWrite32 (*Output, 0, 23, Value); > return Value; > } > > @@ -135,12 +143,14 @@ WriteUnaligned24 ( > UINT32 > EFIAPI > ReadUnaligned32 ( > - IN CONST UINT32 *Buffer > + IN CONST VOID *Buffer > ) > { > + CONST UINT32 *Input = Buffer; > + > ASSERT (Buffer != NULL); > > - return *Buffer; > + return *Input; > } > > /** > @@ -161,13 +171,15 @@ ReadUnaligned32 ( > UINT32 > EFIAPI > WriteUnaligned32 ( > - OUT UINT32 *Buffer, > + OUT VOID *Buffer, > IN UINT32 Value > ) > { > + UINT32 *Output = Buffer; > + > ASSERT (Buffer != NULL); > > - return *Buffer = Value; > + return *Output = Value; > } > > /** > @@ -186,12 +198,14 @@ WriteUnaligned32 ( > UINT64 > EFIAPI > ReadUnaligned64 ( > - IN CONST UINT64 *Buffer > + IN CONST VOID *Buffer > ) > { > + CONST UINT64 *Input = Buffer; > + > ASSERT (Buffer != NULL); > > - return *Buffer; > + return *Input; > } > > /** > @@ -212,11 +226,13 @@ ReadUnaligned64 ( > UINT64 > EFIAPI > WriteUnaligned64 ( > - OUT UINT64 *Buffer, > + OUT VOID *Buffer, > IN UINT64 Value > ) > { > + UINT64 *Output = Buffer; > + > ASSERT (Buffer != NULL); > > - return *Buffer = Value; > + return *Output = Value; > } > -- > 2.1.4 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
