On Thu, May 12, 2016 at 10:29:34PM +0000, Kinney, Michael D wrote: > 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.
Sure, but that does not make it valid C. Specifying an input argument to be a UINT32 * at least in ARM, AArch64 and IPF land means that the programmer provides an explicit guarantee that the pointer references a naturally aligned location. Also, the reason that edk2 compiles without warnings is that code in the tree makes (also invalid) casts at the call sites to these functions in order to get rid of said warnings. See for example MdeModulePkg/Universal/PcatSingleSegmentPciCfg2Pei/PciCfg2.c (A naive grep throws up 129 instances of explicit casts on lines calling a ReadUnaligned function and 211 instances of ones calling a WriteUnaligned function.) > 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. Understood - PatchCheck.py didn't warn me about that :) Regards, Leif > 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
