| Message ID | 20171023025016.3088-1-programmingkidx@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [libfdt,v3] implement strnlen for systems that need it | expand |
On Sun, Oct 22, 2017 at 10:50:16PM -0400, John Arbuckle wrote: > Prior the Mac OS 10.7, the function strnlen() was not available. This patch > implements strnlen() on Mac OS X versions that are below 10.7. > > Signed-off-by: John Arbuckle <programmingkidx@gmail.com> > --- > v3 changes: > - Replaced loop with memchr() > > v2 changes: > - Simplified the code to make it static inline'ed > - Changed the type of count to size_t > > libfdt/libfdt_env.h | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/libfdt/libfdt_env.h b/libfdt/libfdt_env.h > index 952056c..d43b66b 100644 > --- a/libfdt/libfdt_env.h > +++ b/libfdt/libfdt_env.h > @@ -109,4 +109,28 @@ static inline fdt64_t cpu_to_fdt64(uint64_t x) > #undef CPU_TO_FDT16 > #undef EXTRACT_BYTE > > +#ifdef __APPLE__ > +#include <AvailabilityMacros.h> > + > +#define MAC_OS_X_VERSION_10_7 1070 Apple has already defined MAC_OS_X_VERSION_10_7 here: https://opensource.apple.com/source/xnu/xnu-1699.24.8/EXTERNAL_HEADERS/AvailabilityMacros.h To avoid a compiler warning, please use: #ifndef MAC_OS_X_VERSION_10_7 #define MAC_OS_X_VERSION_10_7 1070 #endif > +/* strnlen() is not available on Mac OS < 10.7 */ > +# if (MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7) Does this cover the case where MAC_OS_X_VERSION_MAX_ALLOWED is set to <1070 on a 10.7+ build machine? It's possible that the <string.h> header would define strnlen() and your code redefines the function (compiler error). It would be best to check how <string.h>, <Availability.h>, and <AvailabilityMacros.h> work to make sure that all cases are handled. I don't have access to a Mac right now, sorry. Perhaps this approach works better: # ifndef MAC_OS_X_VERSION_10_7
On 23 October 2017 at 17:09, Stefan Hajnoczi <stefanha@gmail.com> wrote: >> +/* strnlen() is not available on Mac OS < 10.7 */ >> +# if (MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7) > > Does this cover the case where MAC_OS_X_VERSION_MAX_ALLOWED is set to > <1070 on a 10.7+ build machine? It's possible that the <string.h> > header would define strnlen() and your code redefines the function > (compiler error). In that case you don't want to use the strnlen() declaration from the header, you want the inline somehow, because even if the declaration is present and using it doesn't fail compile the definition won't be around at runtime. > It would be best to check how <string.h>, <Availability.h>, and > <AvailabilityMacros.h> work to make sure that all cases are handled. I > don't have access to a Mac right now, sorry. It uses the clang 'attribute availability' syntax: https://clang.llvm.org/docs/AttributeReference.html#availability thanks -- PMM
> On Oct 23, 2017, at 12:09 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > > On Sun, Oct 22, 2017 at 10:50:16PM -0400, John Arbuckle wrote: >> Prior the Mac OS 10.7, the function strnlen() was not available. This patch >> implements strnlen() on Mac OS X versions that are below 10.7. >> >> Signed-off-by: John Arbuckle <programmingkidx@gmail.com> >> --- >> v3 changes: >> - Replaced loop with memchr() >> >> v2 changes: >> - Simplified the code to make it static inline'ed >> - Changed the type of count to size_t >> >> libfdt/libfdt_env.h | 24 ++++++++++++++++++++++++ >> 1 file changed, 24 insertions(+) >> >> diff --git a/libfdt/libfdt_env.h b/libfdt/libfdt_env.h >> index 952056c..d43b66b 100644 >> --- a/libfdt/libfdt_env.h >> +++ b/libfdt/libfdt_env.h >> @@ -109,4 +109,28 @@ static inline fdt64_t cpu_to_fdt64(uint64_t x) >> #undef CPU_TO_FDT16 >> #undef EXTRACT_BYTE >> >> +#ifdef __APPLE__ >> +#include <AvailabilityMacros.h> >> + >> +#define MAC_OS_X_VERSION_10_7 1070 > > Apple has already defined MAC_OS_X_VERSION_10_7 here: > https://opensource.apple.com/source/xnu/xnu-1699.24.8/EXTERNAL_HEADERS/AvailabilityMacros.h > > To avoid a compiler warning, please use: > > #ifndef MAC_OS_X_VERSION_10_7 > #define MAC_OS_X_VERSION_10_7 1070 > #endif Sounds logical. > >> +/* strnlen() is not available on Mac OS < 10.7 */ >> +# if (MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7) > > Does this cover the case where MAC_OS_X_VERSION_MAX_ALLOWED is set to > <1070 on a 10.7+ build machine? It's possible that the <string.h> > header would define strnlen() and your code redefines the function > (compiler error). If MAC_OS_X_VERSION_MAX_ALLOWED is equal to 1070 then this code would not be executed. This branch would only be taken if MAC_OS_X_VERSION_MAX_ALLOWED is less than 1070. So there would not be a compiler error. > > It would be best to check how <string.h>, <Availability.h>, and > <AvailabilityMacros.h> work to make sure that all cases are handled. I > don't have access to a Mac right now, sorry. > > Perhaps this approach works better: > > # ifndef MAC_OS_X_VERSION_10_7 I think you are saying I should remove the "#ifdef __APPLE__" code and just check to see if MAC_OS_X_VERSION_10_7 is defined. That might work on Mac OS 10.6 and 10.5, but it would probably cause problems with non-Mac-OS platforms.
> On Oct 23, 2017, at 12:27 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > > On 23 October 2017 at 17:09, Stefan Hajnoczi <stefanha@gmail.com> wrote: >>> +/* strnlen() is not available on Mac OS < 10.7 */ >>> +# if (MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7) >> >> Does this cover the case where MAC_OS_X_VERSION_MAX_ALLOWED is set to >> <1070 on a 10.7+ build machine? It's possible that the <string.h> >> header would define strnlen() and your code redefines the function >> (compiler error). > I was operating under the assumption that MAC_OS_X_VERSION_MAX_ALLOWED would equal the version of the host. After making this little test program: #include <stdio.h> #include <AvailabilityMacros.h> int main(int argc, char *argv[]) { printf("value = %d\n", MAC_OS_X_VERSION_MAX_ALLOWED); } It reports: "value = 101204" on Mac OS 10.12.6 (I'm not sure why there is a 04) and "Value = 1068" on Mac OS 10.6.8 Is using MAC_OS_X_VERSION_MAX_ALLOWED not a reliable macro to use to test for the version of the Mac OS? The ui/cocoa.m file seems to use it and have no problems. I don't think we have to worry about MAC_OS_X_VERSION_MAX_ALLOWED being set to less than 1070 on Mac OS 10.7 and up. > In that case you don't want to use the strnlen() declaration > from the header, you want the inline somehow, because even if > the declaration is present and using it doesn't fail compile > the definition won't be around at runtime. > >> It would be best to check how <string.h>, <Availability.h>, and >> <AvailabilityMacros.h> work to make sure that all cases are handled. I >> don't have access to a Mac right now, sorry. > > It uses the clang 'attribute availability' syntax: > https://clang.llvm.org/docs/AttributeReference.html#availability This feature appears to be a clang/gcc-only feature. Using it would mean making this code compiler locked. The Device Tree Compiler project (that this code belongs to) is made by IBM personnel. They might want to be able to use other compilers including their own IBM XL C compiler to compile this project. Even if that part of the code is only to run on Mac OS X I still would like to keep the code generic enough for any compiler to be able to build the Device Tree Compiler project.
On 24 October 2017 at 04:45, Programmingkid <programmingkidx@gmail.com> wrote: > I was operating under the assumption that MAC_OS_X_VERSION_MAX_ALLOWED > would equal the version of the host. It indicates the highest version of OSX whose features the program being compiled is allowed to use. That isn't necessarily the same as the highest version of OSX the SDK we're compiling against supports -- you can be building against a 10.9-aware SDK and tell it "only use features up to and including those on 10.7." > After making this little test program: You don't need to test this kind of thing, it is documented, and in general reading the documentation is more reliable and more informative than testing. > Is using MAC_OS_X_VERSION_MAX_ALLOWED not a reliable macro to > use to test for the version of the Mac OS? The ui/cocoa.m file > seems to use it and have no problems. QEMU's usage differs in two ways: (1) we just use the macros to #ifdef out code that would be trying to use functions that don't exist in the older versions, which is exactly what it's intended for. We don't try to implement our own versions of those functions under the same names (2) we control QEMU's build process and so we can guarantee that we aren't using the "build on a 10.9 SDK but make sure you don't use features that aren't in 10.7" functionality. libfdt is a library that can be included in many other programs, so we can't make that assumption. >> It uses the clang 'attribute availability' syntax: >> https://clang.llvm.org/docs/AttributeReference.html#availability > > This feature appears to be a clang/gcc-only feature. This was a reply to Stefan's question about how the OSX availability headers are implemented. (As it happens the OSX headers do have an "if not clang then just #define away the availability attributes" codepath.) thanks -- PMM
On Mon, Oct 23, 2017 at 11:13:13PM -0400, Programmingkid wrote: > > > On Oct 23, 2017, at 12:09 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > > > > On Sun, Oct 22, 2017 at 10:50:16PM -0400, John Arbuckle wrote: > >> Prior the Mac OS 10.7, the function strnlen() was not available. This patch > >> implements strnlen() on Mac OS X versions that are below 10.7. > >> > >> Signed-off-by: John Arbuckle <programmingkidx@gmail.com> > >> --- > >> v3 changes: > >> - Replaced loop with memchr() > >> > >> v2 changes: > >> - Simplified the code to make it static inline'ed > >> - Changed the type of count to size_t > >> > >> libfdt/libfdt_env.h | 24 ++++++++++++++++++++++++ > >> 1 file changed, 24 insertions(+) > >> > >> diff --git a/libfdt/libfdt_env.h b/libfdt/libfdt_env.h > >> index 952056c..d43b66b 100644 > >> --- a/libfdt/libfdt_env.h > >> +++ b/libfdt/libfdt_env.h > >> @@ -109,4 +109,28 @@ static inline fdt64_t cpu_to_fdt64(uint64_t x) > >> #undef CPU_TO_FDT16 > >> #undef EXTRACT_BYTE > >> > >> +#ifdef __APPLE__ > >> +#include <AvailabilityMacros.h> > >> + > >> +#define MAC_OS_X_VERSION_10_7 1070 > > > > Apple has already defined MAC_OS_X_VERSION_10_7 here: > > https://opensource.apple.com/source/xnu/xnu-1699.24.8/EXTERNAL_HEADERS/AvailabilityMacros.h > > > > To avoid a compiler warning, please use: > > > > #ifndef MAC_OS_X_VERSION_10_7 > > #define MAC_OS_X_VERSION_10_7 1070 > > #endif > > Sounds logical. > > > > >> +/* strnlen() is not available on Mac OS < 10.7 */ > >> +# if (MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7) > > > > Does this cover the case where MAC_OS_X_VERSION_MAX_ALLOWED is set to > > <1070 on a 10.7+ build machine? It's possible that the <string.h> > > header would define strnlen() and your code redefines the function > > (compiler error). > > If MAC_OS_X_VERSION_MAX_ALLOWED is equal to 1070 then this code would not be executed. This branch would only be taken if MAC_OS_X_VERSION_MAX_ALLOWED is less than 1070. So there would not be a compiler error. > > > > > It would be best to check how <string.h>, <Availability.h>, and > > <AvailabilityMacros.h> work to make sure that all cases are handled. I > > don't have access to a Mac right now, sorry. > > > > Perhaps this approach works better: > > > > # ifndef MAC_OS_X_VERSION_10_7 > > I think you are saying I should remove the "#ifdef __APPLE__" code and just check to see if MAC_OS_X_VERSION_10_7 is defined. That might work on Mac OS 10.6 and 10.5, but it would probably cause problems with non-Mac-OS platforms. No, the ifndef is still "indented" and it would be like this: #ifdef __APPLE_ ... # ifndef MAC_OS_X_VERSION_10_7
On Mon, Oct 23, 2017 at 05:27:26PM +0100, Peter Maydell wrote: > On 23 October 2017 at 17:09, Stefan Hajnoczi <stefanha@gmail.com> wrote: > >> +/* strnlen() is not available on Mac OS < 10.7 */ > >> +# if (MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7) > > > > Does this cover the case where MAC_OS_X_VERSION_MAX_ALLOWED is set to > > <1070 on a 10.7+ build machine? It's possible that the <string.h> > > header would define strnlen() and your code redefines the function > > (compiler error). > > In that case you don't want to use the strnlen() declaration > from the header, you want the inline somehow, because even if > the declaration is present and using it doesn't fail compile > the definition won't be around at runtime. Perhaps one way around this case is to: # if !defined(MAC_OS_X_VERSION_10_7) || (MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7) # include <string.h> /* make sure it isn't included again */ # define strnlen fdt_strnlen static inline fdt_strnlen(...) { ... } This way the symbol strnlen() isn't used and the system headers cannot cause problems. Stefan
> On Oct 24, 2017, at 8:09 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > > On Mon, Oct 23, 2017 at 11:13:13PM -0400, Programmingkid wrote: >> >>> On Oct 23, 2017, at 12:09 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: >>> >>> On Sun, Oct 22, 2017 at 10:50:16PM -0400, John Arbuckle wrote: >>>> Prior the Mac OS 10.7, the function strnlen() was not available. This patch >>>> implements strnlen() on Mac OS X versions that are below 10.7. >>>> >>>> Signed-off-by: John Arbuckle <programmingkidx@gmail.com> >>>> --- >>>> v3 changes: >>>> - Replaced loop with memchr() >>>> >>>> v2 changes: >>>> - Simplified the code to make it static inline'ed >>>> - Changed the type of count to size_t >>>> >>>> libfdt/libfdt_env.h | 24 ++++++++++++++++++++++++ >>>> 1 file changed, 24 insertions(+) >>>> >>>> diff --git a/libfdt/libfdt_env.h b/libfdt/libfdt_env.h >>>> index 952056c..d43b66b 100644 >>>> --- a/libfdt/libfdt_env.h >>>> +++ b/libfdt/libfdt_env.h >>>> @@ -109,4 +109,28 @@ static inline fdt64_t cpu_to_fdt64(uint64_t x) >>>> #undef CPU_TO_FDT16 >>>> #undef EXTRACT_BYTE >>>> >>>> +#ifdef __APPLE__ >>>> +#include <AvailabilityMacros.h> >>>> + >>>> +#define MAC_OS_X_VERSION_10_7 1070 >>> >>> Apple has already defined MAC_OS_X_VERSION_10_7 here: >>> https://opensource.apple.com/source/xnu/xnu-1699.24.8/EXTERNAL_HEADERS/AvailabilityMacros.h >>> >>> To avoid a compiler warning, please use: >>> >>> #ifndef MAC_OS_X_VERSION_10_7 >>> #define MAC_OS_X_VERSION_10_7 1070 >>> #endif >> >> Sounds logical. >> >>> >>>> +/* strnlen() is not available on Mac OS < 10.7 */ >>>> +# if (MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7) >>> >>> Does this cover the case where MAC_OS_X_VERSION_MAX_ALLOWED is set to >>> <1070 on a 10.7+ build machine? It's possible that the <string.h> >>> header would define strnlen() and your code redefines the function >>> (compiler error). >> >> If MAC_OS_X_VERSION_MAX_ALLOWED is equal to 1070 then this code would not be executed. This branch would only be taken if MAC_OS_X_VERSION_MAX_ALLOWED is less than 1070. So there would not be a compiler error. >> >>> >>> It would be best to check how <string.h>, <Availability.h>, and >>> <AvailabilityMacros.h> work to make sure that all cases are handled. I >>> don't have access to a Mac right now, sorry. >>> >>> Perhaps this approach works better: >>> >>> # ifndef MAC_OS_X_VERSION_10_7 >> >> I think you are saying I should remove the "#ifdef __APPLE__" code and just check to see if MAC_OS_X_VERSION_10_7 is defined. That might work on Mac OS 10.6 and 10.5, but it would probably cause problems with non-Mac-OS platforms. > > No, the ifndef is still "indented" and it would be like this: > > #ifdef __APPLE_ > ... > # ifndef MAC_OS_X_VERSION_10_7 I see. This is a more compact way of doing things.
> On Oct 24, 2017, at 8:18 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > > On Mon, Oct 23, 2017 at 05:27:26PM +0100, Peter Maydell wrote: >> On 23 October 2017 at 17:09, Stefan Hajnoczi <stefanha@gmail.com> wrote: >>>> +/* strnlen() is not available on Mac OS < 10.7 */ >>>> +# if (MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7) >>> >>> Does this cover the case where MAC_OS_X_VERSION_MAX_ALLOWED is set to >>> <1070 on a 10.7+ build machine? It's possible that the <string.h> >>> header would define strnlen() and your code redefines the function >>> (compiler error). >> >> In that case you don't want to use the strnlen() declaration >> from the header, you want the inline somehow, because even if >> the declaration is present and using it doesn't fail compile >> the definition won't be around at runtime. > > Perhaps one way around this case is to: > > # if !defined(MAC_OS_X_VERSION_10_7) || (MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7) > # include <string.h> /* make sure it isn't included again */ > # define strnlen fdt_strnlen > static inline fdt_strnlen(...) { ... } > > This way the symbol strnlen() isn't used and the system headers cannot > cause problems. I really like this idea. I will implement it in my next patch. Thanks!
diff --git a/libfdt/libfdt_env.h b/libfdt/libfdt_env.h index 952056c..d43b66b 100644 --- a/libfdt/libfdt_env.h +++ b/libfdt/libfdt_env.h @@ -109,4 +109,28 @@ static inline fdt64_t cpu_to_fdt64(uint64_t x) #undef CPU_TO_FDT16 #undef EXTRACT_BYTE +#ifdef __APPLE__ +#include <AvailabilityMacros.h> + +#define MAC_OS_X_VERSION_10_7 1070 + +/* strnlen() is not available on Mac OS < 10.7 */ +# if (MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7) + +/* + * strnlen: returns the length of a string or max_count - which ever is smallest + * Input 1 string: the string whose size is to be determined + * Input 2 max_count: the maximum value returned by this function + * Output: length of the string or max_count (the smallest of the two) + */ +static inline size_t strnlen(const char *string, size_t max_count) +{ + const char *p = memchr(string, 0, max_count); + return p ? p - string : max_count; +} + +#endif /* (MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7) */ + +#endif /* __APPLE__ */ + #endif /* _LIBFDT_ENV_H */
Prior the Mac OS 10.7, the function strnlen() was not available. This patch implements strnlen() on Mac OS X versions that are below 10.7. Signed-off-by: John Arbuckle <programmingkidx@gmail.com> --- v3 changes: - Replaced loop with memchr() v2 changes: - Simplified the code to make it static inline'ed - Changed the type of count to size_t libfdt/libfdt_env.h | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)