| Message ID | 20181130224537.18936-7-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | BaseTools: get rid of MAX_UINTN | expand |
On 30/11/18 23:45, Ard Biesheuvel wrote: > The maximum value that can be represented by the native word size > of the *target* should be irrelevant when compiling tools that > run on the build *host*. So drop the definition of MAX_UINTN, now > that we no longer use it. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Reviewed-by: Jaben Carsey <jaben.carsey@intel.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > BaseTools/Source/C/Common/CommonLib.h | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/BaseTools/Source/C/Common/CommonLib.h b/BaseTools/Source/C/Common/CommonLib.h > index 6930d9227b87..b1c6c00a3478 100644 > --- a/BaseTools/Source/C/Common/CommonLib.h > +++ b/BaseTools/Source/C/Common/CommonLib.h > @@ -22,7 +22,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > > #define MAX_LONG_FILE_PATH 500 > > -#define MAX_UINTN MAX_ADDRESS > #define MAX_UINT64 ((UINT64)0xFFFFFFFFFFFFFFFFULL) > #define MAX_UINT16 ((UINT16)0xFFFF) > #define MAX_UINT8 ((UINT8)0xFF) >
On 11/30/18 23:45, Ard Biesheuvel wrote: > The maximum value that can be represented by the native word size > of the *target* should be irrelevant when compiling tools that > run on the build *host*. So drop the definition of MAX_UINTN, now > that we no longer use it. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Reviewed-by: Jaben Carsey <jaben.carsey@intel.com> > --- > BaseTools/Source/C/Common/CommonLib.h | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/BaseTools/Source/C/Common/CommonLib.h b/BaseTools/Source/C/Common/CommonLib.h > index 6930d9227b87..b1c6c00a3478 100644 > --- a/BaseTools/Source/C/Common/CommonLib.h > +++ b/BaseTools/Source/C/Common/CommonLib.h > @@ -22,7 +22,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > > #define MAX_LONG_FILE_PATH 500 > > -#define MAX_UINTN MAX_ADDRESS > #define MAX_UINT64 ((UINT64)0xFFFFFFFFFFFFFFFFULL) > #define MAX_UINT16 ((UINT16)0xFFFF) > #define MAX_UINT8 ((UINT8)0xFF) > Reviewed-by: Laszlo Ersek <lersek@redhat.com> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Not sure why you'd take that out when someone using UINTN for variables may want to use MAX_UINTN ? Future may be different. On Mon, Dec 3, 2018 at 5:08 AM Laszlo Ersek <lersek@redhat.com> wrote: > On 11/30/18 23:45, Ard Biesheuvel wrote: > > The maximum value that can be represented by the native word size > > of the *target* should be irrelevant when compiling tools that > > run on the build *host*. So drop the definition of MAX_UINTN, now > > that we no longer use it. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > Reviewed-by: Jaben Carsey <jaben.carsey@intel.com> > > --- > > BaseTools/Source/C/Common/CommonLib.h | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/BaseTools/Source/C/Common/CommonLib.h > b/BaseTools/Source/C/Common/CommonLib.h > > index 6930d9227b87..b1c6c00a3478 100644 > > --- a/BaseTools/Source/C/Common/CommonLib.h > > +++ b/BaseTools/Source/C/Common/CommonLib.h > > @@ -22,7 +22,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, > EITHER EXPRESS OR IMPLIED. > > > > #define MAX_LONG_FILE_PATH 500 > > > > -#define MAX_UINTN MAX_ADDRESS > > #define MAX_UINT64 ((UINT64)0xFFFFFFFFFFFFFFFFULL) > > #define MAX_UINT16 ((UINT16)0xFFFF) > > #define MAX_UINT8 ((UINT8)0xFF) > > > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 12/11/18 08:11, David F. wrote: > Not sure why you'd take that out when someone using UINTN for variables may > want to use MAX_UINTN ? Future may be different. The UINTN type comes from the UEFI spec: Unsigned value of native width. (4 bytes on supported 32-bit processor instructions, 8 bytes on supported 64-bit processor instructions, 16 bytes on supported 128-bit processor instructions) In this sense, "native" refers to the firmware execution environment. The firmware execution environment need not have anything in common with the build environment. (You can build 32-bit ARM firmware on X64 hosts.) In such a scenario, using UINTN *at all* is fraught with misunderstandings. It *would* be possible to use UINTN as it applies to the build (= hosted) environment, and in that sense MAX_UINTN would also be possible to define. However, the code being removed (= defining MAX_UINTN as MAX_ADDRESS) proves that that approach would be very easy to misunderstand and misuse. People could easily mistake it for applying to the firmware execution environment. UINT32 and UINT64 are not affected by this ambiguity. Optimally, given that the build utilities target a hosted C runtime, they should use standard C types, such as "unsigned int", or e.g. "uint32_t". Together with standard C macros expressing limits, such as UINT_MAX (from <limits.h>) and UINT32_MAX (from <stdint.h>). Clearly no-one has capacity to clean up BaseTools like this. For starters, we should at least remove whatever actively causes confusion. Thanks, Laszlo > On Mon, Dec 3, 2018 at 5:08 AM Laszlo Ersek <lersek@redhat.com> wrote: > >> On 11/30/18 23:45, Ard Biesheuvel wrote: >>> The maximum value that can be represented by the native word size >>> of the *target* should be irrelevant when compiling tools that >>> run on the build *host*. So drop the definition of MAX_UINTN, now >>> that we no longer use it. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.1 >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> Reviewed-by: Jaben Carsey <jaben.carsey@intel.com> >>> --- >>> BaseTools/Source/C/Common/CommonLib.h | 1 - >>> 1 file changed, 1 deletion(-) >>> >>> diff --git a/BaseTools/Source/C/Common/CommonLib.h >> b/BaseTools/Source/C/Common/CommonLib.h >>> index 6930d9227b87..b1c6c00a3478 100644 >>> --- a/BaseTools/Source/C/Common/CommonLib.h >>> +++ b/BaseTools/Source/C/Common/CommonLib.h >>> @@ -22,7 +22,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, >> EITHER EXPRESS OR IMPLIED. >>> >>> #define MAX_LONG_FILE_PATH 500 >>> >>> -#define MAX_UINTN MAX_ADDRESS >>> #define MAX_UINT64 ((UINT64)0xFFFFFFFFFFFFFFFFULL) >>> #define MAX_UINT16 ((UINT16)0xFFFF) >>> #define MAX_UINT8 ((UINT8)0xFF) >>> >> >> Reviewed-by: Laszlo Ersek <lersek@redhat.com> >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel >> > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
I don't know, to me it's very clear that UINTN is talking about the target, just like size_t would be. There are/were a bunch of API's using UINTN so using UINTN was desirable, and where needed UINTN_MAX. I just don't see an advantage to removing it. Do see disadvantage to removing it for breaking existing code and for those that want the "native" (best/fasted/most efficient) int size for the processor (similar again to size_t) On Tue, Dec 11, 2018 at 7:46 AM Laszlo Ersek <lersek@redhat.com> wrote: > On 12/11/18 08:11, David F. wrote: > > Not sure why you'd take that out when someone using UINTN for variables > may > > want to use MAX_UINTN ? Future may be different. > > The UINTN type comes from the UEFI spec: > > Unsigned value of native width. (4 bytes on supported 32-bit > processor instructions, 8 bytes on supported 64-bit processor > instructions, 16 bytes on supported 128-bit processor instructions) > > In this sense, "native" refers to the firmware execution environment. > The firmware execution environment need not have anything in common with > the build environment. (You can build 32-bit ARM firmware on X64 hosts.) > In such a scenario, using UINTN *at all* is fraught with > misunderstandings. It *would* be possible to use UINTN as it applies to > the build (= hosted) environment, and in that sense MAX_UINTN would also > be possible to define. However, the code being removed (= defining > MAX_UINTN as MAX_ADDRESS) proves that that approach would be very easy > to misunderstand and misuse. People could easily mistake it for applying > to the firmware execution environment. > > UINT32 and UINT64 are not affected by this ambiguity. > > Optimally, given that the build utilities target a hosted C runtime, > they should use standard C types, such as "unsigned int", or e.g. > "uint32_t". Together with standard C macros expressing limits, such as > UINT_MAX (from <limits.h>) and UINT32_MAX (from <stdint.h>). > > Clearly no-one has capacity to clean up BaseTools like this. For > starters, we should at least remove whatever actively causes confusion. > > Thanks, > Laszlo > > > On Mon, Dec 3, 2018 at 5:08 AM Laszlo Ersek <lersek@redhat.com> wrote: > > > >> On 11/30/18 23:45, Ard Biesheuvel wrote: > >>> The maximum value that can be represented by the native word size > >>> of the *target* should be irrelevant when compiling tools that > >>> run on the build *host*. So drop the definition of MAX_UINTN, now > >>> that we no longer use it. > >>> > >>> Contributed-under: TianoCore Contribution Agreement 1.1 > >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > >>> Reviewed-by: Jaben Carsey <jaben.carsey@intel.com> > >>> --- > >>> BaseTools/Source/C/Common/CommonLib.h | 1 - > >>> 1 file changed, 1 deletion(-) > >>> > >>> diff --git a/BaseTools/Source/C/Common/CommonLib.h > >> b/BaseTools/Source/C/Common/CommonLib.h > >>> index 6930d9227b87..b1c6c00a3478 100644 > >>> --- a/BaseTools/Source/C/Common/CommonLib.h > >>> +++ b/BaseTools/Source/C/Common/CommonLib.h > >>> @@ -22,7 +22,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, > >> EITHER EXPRESS OR IMPLIED. > >>> > >>> #define MAX_LONG_FILE_PATH 500 > >>> > >>> -#define MAX_UINTN MAX_ADDRESS > >>> #define MAX_UINT64 ((UINT64)0xFFFFFFFFFFFFFFFFULL) > >>> #define MAX_UINT16 ((UINT16)0xFFFF) > >>> #define MAX_UINT8 ((UINT8)0xFF) > >>> > >> > >> Reviewed-by: Laszlo Ersek <lersek@redhat.com> > >> _______________________________________________ > >> edk2-devel mailing list > >> edk2-devel@lists.01.org > >> https://lists.01.org/mailman/listinfo/edk2-devel > >> > > > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On Tue, 11 Dec 2018 at 23:53, David F. <df7729@gmail.com> wrote: > > I don't know, to me it's very clear that UINTN is talking about the target, just like size_t would be. > But which target? This change is against the source of the BaseTools, which are host tools that can be used to build a single target image consisting of 32-bit and 64-bit modules. So which size is the native size in this case? > There are/were a bunch of API's using UINTN so using UINTN was desirable, and where needed UINTN_MAX. > > I just don't see an advantage to removing it. Do see disadvantage to removing it for breaking existing code and for those that want the "native" (best/fasted/most efficient) int size for the processor (similar again to size_t) > > On Tue, Dec 11, 2018 at 7:46 AM Laszlo Ersek <lersek@redhat.com> wrote: >> >> On 12/11/18 08:11, David F. wrote: >> > Not sure why you'd take that out when someone using UINTN for variables may >> > want to use MAX_UINTN ? Future may be different. >> >> The UINTN type comes from the UEFI spec: >> >> Unsigned value of native width. (4 bytes on supported 32-bit >> processor instructions, 8 bytes on supported 64-bit processor >> instructions, 16 bytes on supported 128-bit processor instructions) >> >> In this sense, "native" refers to the firmware execution environment. >> The firmware execution environment need not have anything in common with >> the build environment. (You can build 32-bit ARM firmware on X64 hosts.) >> In such a scenario, using UINTN *at all* is fraught with >> misunderstandings. It *would* be possible to use UINTN as it applies to >> the build (= hosted) environment, and in that sense MAX_UINTN would also >> be possible to define. However, the code being removed (= defining >> MAX_UINTN as MAX_ADDRESS) proves that that approach would be very easy >> to misunderstand and misuse. People could easily mistake it for applying >> to the firmware execution environment. >> >> UINT32 and UINT64 are not affected by this ambiguity. >> >> Optimally, given that the build utilities target a hosted C runtime, >> they should use standard C types, such as "unsigned int", or e.g. >> "uint32_t". Together with standard C macros expressing limits, such as >> UINT_MAX (from <limits.h>) and UINT32_MAX (from <stdint.h>). >> >> Clearly no-one has capacity to clean up BaseTools like this. For >> starters, we should at least remove whatever actively causes confusion. >> >> Thanks, >> Laszlo >> >> > On Mon, Dec 3, 2018 at 5:08 AM Laszlo Ersek <lersek@redhat.com> wrote: >> > >> >> On 11/30/18 23:45, Ard Biesheuvel wrote: >> >>> The maximum value that can be represented by the native word size >> >>> of the *target* should be irrelevant when compiling tools that >> >>> run on the build *host*. So drop the definition of MAX_UINTN, now >> >>> that we no longer use it. >> >>> >> >>> Contributed-under: TianoCore Contribution Agreement 1.1 >> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> >>> Reviewed-by: Jaben Carsey <jaben.carsey@intel.com> >> >>> --- >> >>> BaseTools/Source/C/Common/CommonLib.h | 1 - >> >>> 1 file changed, 1 deletion(-) >> >>> >> >>> diff --git a/BaseTools/Source/C/Common/CommonLib.h >> >> b/BaseTools/Source/C/Common/CommonLib.h >> >>> index 6930d9227b87..b1c6c00a3478 100644 >> >>> --- a/BaseTools/Source/C/Common/CommonLib.h >> >>> +++ b/BaseTools/Source/C/Common/CommonLib.h >> >>> @@ -22,7 +22,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, >> >> EITHER EXPRESS OR IMPLIED. >> >>> >> >>> #define MAX_LONG_FILE_PATH 500 >> >>> >> >>> -#define MAX_UINTN MAX_ADDRESS >> >>> #define MAX_UINT64 ((UINT64)0xFFFFFFFFFFFFFFFFULL) >> >>> #define MAX_UINT16 ((UINT16)0xFFFF) >> >>> #define MAX_UINT8 ((UINT8)0xFF) >> >>> >> >> >> >> Reviewed-by: Laszlo Ersek <lersek@redhat.com> >> >> _______________________________________________ >> >> edk2-devel mailing list >> >> edk2-devel@lists.01.org >> >> https://lists.01.org/mailman/listinfo/edk2-devel >> >> >> > >> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
I missed that it was for the build-tool source itself and not for the targets that are built using edk2 and the API itself. On Tue, Dec 11, 2018 at 2:55 PM Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On Tue, 11 Dec 2018 at 23:53, David F. <df7729@gmail.com> wrote: > > > > I don't know, to me it's very clear that UINTN is talking about the > target, just like size_t would be. > > > > But which target? This change is against the source of the BaseTools, > which are host tools that can be used to build a single target image > consisting of 32-bit and 64-bit modules. So which size is the native > size in this case? > > > There are/were a bunch of API's using UINTN so using UINTN was > desirable, and where needed UINTN_MAX. > > > > I just don't see an advantage to removing it. Do see disadvantage to > removing it for breaking existing code and for those that want the "native" > (best/fasted/most efficient) int size for the processor (similar again to > size_t) > > > > On Tue, Dec 11, 2018 at 7:46 AM Laszlo Ersek <lersek@redhat.com> wrote: > >> > >> On 12/11/18 08:11, David F. wrote: > >> > Not sure why you'd take that out when someone using UINTN for > variables may > >> > want to use MAX_UINTN ? Future may be different. > >> > >> The UINTN type comes from the UEFI spec: > >> > >> Unsigned value of native width. (4 bytes on supported 32-bit > >> processor instructions, 8 bytes on supported 64-bit processor > >> instructions, 16 bytes on supported 128-bit processor instructions) > >> > >> In this sense, "native" refers to the firmware execution environment. > >> The firmware execution environment need not have anything in common with > >> the build environment. (You can build 32-bit ARM firmware on X64 hosts.) > >> In such a scenario, using UINTN *at all* is fraught with > >> misunderstandings. It *would* be possible to use UINTN as it applies to > >> the build (= hosted) environment, and in that sense MAX_UINTN would also > >> be possible to define. However, the code being removed (= defining > >> MAX_UINTN as MAX_ADDRESS) proves that that approach would be very easy > >> to misunderstand and misuse. People could easily mistake it for applying > >> to the firmware execution environment. > >> > >> UINT32 and UINT64 are not affected by this ambiguity. > >> > >> Optimally, given that the build utilities target a hosted C runtime, > >> they should use standard C types, such as "unsigned int", or e.g. > >> "uint32_t". Together with standard C macros expressing limits, such as > >> UINT_MAX (from <limits.h>) and UINT32_MAX (from <stdint.h>). > >> > >> Clearly no-one has capacity to clean up BaseTools like this. For > >> starters, we should at least remove whatever actively causes confusion. > >> > >> Thanks, > >> Laszlo > >> > >> > On Mon, Dec 3, 2018 at 5:08 AM Laszlo Ersek <lersek@redhat.com> > wrote: > >> > > >> >> On 11/30/18 23:45, Ard Biesheuvel wrote: > >> >>> The maximum value that can be represented by the native word size > >> >>> of the *target* should be irrelevant when compiling tools that > >> >>> run on the build *host*. So drop the definition of MAX_UINTN, now > >> >>> that we no longer use it. > >> >>> > >> >>> Contributed-under: TianoCore Contribution Agreement 1.1 > >> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > >> >>> Reviewed-by: Jaben Carsey <jaben.carsey@intel.com> > >> >>> --- > >> >>> BaseTools/Source/C/Common/CommonLib.h | 1 - > >> >>> 1 file changed, 1 deletion(-) > >> >>> > >> >>> diff --git a/BaseTools/Source/C/Common/CommonLib.h > >> >> b/BaseTools/Source/C/Common/CommonLib.h > >> >>> index 6930d9227b87..b1c6c00a3478 100644 > >> >>> --- a/BaseTools/Source/C/Common/CommonLib.h > >> >>> +++ b/BaseTools/Source/C/Common/CommonLib.h > >> >>> @@ -22,7 +22,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, > >> >> EITHER EXPRESS OR IMPLIED. > >> >>> > >> >>> #define MAX_LONG_FILE_PATH 500 > >> >>> > >> >>> -#define MAX_UINTN MAX_ADDRESS > >> >>> #define MAX_UINT64 ((UINT64)0xFFFFFFFFFFFFFFFFULL) > >> >>> #define MAX_UINT16 ((UINT16)0xFFFF) > >> >>> #define MAX_UINT8 ((UINT8)0xFF) > >> >>> > >> >> > >> >> Reviewed-by: Laszlo Ersek <lersek@redhat.com> > >> >> _______________________________________________ > >> >> edk2-devel mailing list > >> >> edk2-devel@lists.01.org > >> >> https://lists.01.org/mailman/listinfo/edk2-devel > >> >> > >> > > >> > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/BaseTools/Source/C/Common/CommonLib.h b/BaseTools/Source/C/Common/CommonLib.h index 6930d9227b87..b1c6c00a3478 100644 --- a/BaseTools/Source/C/Common/CommonLib.h +++ b/BaseTools/Source/C/Common/CommonLib.h @@ -22,7 +22,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. #define MAX_LONG_FILE_PATH 500 -#define MAX_UINTN MAX_ADDRESS #define MAX_UINT64 ((UINT64)0xFFFFFFFFFFFFFFFFULL) #define MAX_UINT16 ((UINT16)0xFFFF) #define MAX_UINT8 ((UINT8)0xFF)