Bugzilla – Bug 732
Const/printf fixes from NetBSD
Last modified: 2008-10-15 05:05:55
You need to log in before you can comment on or make changes to this bug.
Bug detailed description: -------------------------- acpica-20080829 can't be compiled out-of-the-box with the usual NetBSD kernel GCC flags. This is largely related to the missing use of char *, especially for string constants. Another problem is the use of incorrect printf types when dealing with size_t and ACPI_THREAD_ID. Last, this forces the unsigned char cast for all users of ctype.h functions. They can be implemented without value range validation and GCC warns at least on NetBSD about this as well.
Created an attachment (id=806) [details] Mentioned patch
>This is largely related to the missing use of char *, especially for string constants. Please explain. >Another problem is the use of incorrect printf types when dealing with size_t and ACPI_THREAD_ID. This is a tough one for us, since ACPI_THREAD_ID can be changed to any type the host needs. The patch looks huge. Other users are not having these problems, I suspect you can use some custom flags to compile things.
(In reply to comment #2) > >This is largely related to the missing use of char *, especially for > string constants. > > Please explain. You are violating the type constraints if you pass a string constant to something taking a char *. The patch hunts down all this cases (most the name / path arguments) and as all those cases should not modify the argument, the compiler can now rightfully complain. > >Another problem is the use of incorrect printf types when dealing with size_t > and ACPI_THREAD_ID. > > This is a tough one for us, since ACPI_THREAD_ID can be changed to any type the > host needs. Yes, but in that case ACPI_THREAD_ID_FMT can be overriden as well. More interesting is whether any of the other printfs would change if ACPI_SIZE is not size_t. If that is expected to be usable, explicit casts should be done though. > The patch looks huge. Other users are not having these problems, I suspect you > can use some custom flags to compile things. The biggest reason to submit this patch is to avoid having to do keep the local patches or disable compiler warnings. If it helps and you are interested, I can split the parts (const vs. ACPI_THREAD_ID in printf vs. other printfs vs. ctype), but the majority is the const part.
Please see Bug 717 and the attachment #788 [details] as well.
Please split these out into: 1) ACPICA core (kernel code) 2) ACPICA applications (application code) As well as changes for const, printf, etc. So that we can address them individually. Thanks.
Created an attachment (id=807) [details] Fix ctype usage ISO C requires the argument for the ctype.h macros to be in the range [EOF, 0..255], e.g. unsigned char or EOF. Due to the way the macros are implemented, GCC can complain on NetBSD if signed char is used.
Created an attachment (id=808) [details] Reorder keywords to proper order Newer GCC will warn if link attributes follow the type of a function, so reorder the definitions.
Created an attachment (id=809) [details] Static for local function The given function should either be static or get a prototype.
Created an attachment (id=810) [details] Fix typo in comment.
Created an attachment (id=811) [details] Const propogation Adjust macros that cast to fixed size integers to use const char * as temporary type. Adjust function arguments and return values if string constants for involved as arguments. Use explicitly modifyable storage for the two places that remain (see acpi_os_name_string and aml_debug_string). Remove many now unnecessary casts to char *.
(In reply to comment #4) > Please see Bug 717 and the attachment #788 [details] [details] as well. The issue mentioned in Bug 717 and some other parts of the original change are left out for the moment. ACPI_THREAD_ID is one case where ACPI_SIZE is used for printf, others exist though. The code currently assumes that using integer formatting works, which is bogus. Do we want to have two macros and push them into the format strings or require the existance of long long support and cast to that? long might be an alternative as well, all platforms I know of have sizeof(void *) = sizeof(size_t) = sizeof(ACPI_SIZE) <= sizeof(long).
Unfortunately MS Windows x64 is LLP64 architecture, i.e., sizeof(void *) = sizeof(size_t) = sizeof(ACPI_SIZE) > sizeof(long)
I've fixed the issues mentioned in #7, #8, and #9. As far as the const issues, one big issue is the change to the external ACPICA interfaces to add the ACPI_CONST_STRING type. This is simply pushing const pollution up into the caller's space. I don't see us changing the external interfaces just to add const -- and potentially causing problems for about a dozen different operating systems. I'm afraid that I will follow the old adage "don't fix it if it isn't broken." This code is very mature, over 10 years old, and I don't feel that adding const buys us much if anything -- and can certainly cause many problems for our users. We will continue to look at the ctype and thread_id issues.
(In reply to comment #13) > As far as the const issues, one big issue is the change to the external ACPICA > interfaces to add the ACPI_CONST_STRING type. This is simply pushing const > pollution up into the caller's space. There are two different situations here. Operating Systems that do build with -Werror and the corresponding flags to complain about const casts and those that don't. The impact for the latter is very small. If I haven't forgotten anything, only two prototypes are changed in a way to directly affect the OS glue, AcpiOsSignal and AcpiOsCreateCache. AcpiOsCreateCache should be trivial as the name is always a string constant and must not be modified anyway. Worst case is not different from the current status quo. AcpiOsSignal is more interesting as one user passes a structure, the second a string constant. I don't think it makes much sense to modify the passed structure, but if that is considered useful the string constant should maybe not passed in directly. All other changes really reflect only the use in the code and modification to the memory will result in writing to .text; they only make the interface stricter, without making it incompatible.