[Qemu-devel] [libfdt][PATCH v3] implement strnlen for systems that need it

[Qemu-devel] [libfdt][PATCH v3] implement strnlen for systems that need it

[John Arbuckle]

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
+
+/* 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 */
-- 
2.10.2

Re: [Qemu-devel] [libfdt][PATCH v3] implement strnlen for systems that need it

[Stefan Hajnoczi]

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

Re: [Qemu-devel] [libfdt][PATCH v3] implement strnlen for systems that need it

[Peter Maydell]

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

Re: [Qemu-devel] [libfdt][PATCH v3] implement strnlen for systems that need it

[Programmingkid]

> 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.

Re: [Qemu-devel] [libfdt][PATCH v3] implement strnlen for systems that need it

[Peter Maydell]

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

Re: [Qemu-devel] [libfdt][PATCH v3] implement strnlen for systems that need it

[Stefan Hajnoczi]

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

Re: [Qemu-devel] [libfdt][PATCH v3] implement strnlen for systems that need it

[Programmingkid]

> 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!

Re: [Qemu-devel] [libfdt][PATCH v3] implement strnlen for systems that need it

[Programmingkid]

> 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. 

Re: [Qemu-devel] [libfdt][PATCH v3] implement strnlen for systems that need it

[Stefan Hajnoczi]

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

Re: [Qemu-devel] [libfdt][PATCH v3] implement strnlen for systems that need it

[Programmingkid]

> 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.