On Sat, Mar 31, 2012 at 12:18:27PM +0200, Mark Kettenis wrote:
> > Date: Sat, 31 Mar 2012 01:47:18 +0300
> > From: Paul Irofti <[email protected]>
> >
> > After the report from a few weeks ago I went ahead and fixed most (if
> > not all) of the signed integer usages in the AML parser.
> >
> > Please have a look at this diff, test it thoroughly and comment/okay it.
>
> So you ran into the same issue that -1 has this special meaning where
> I decided to go for a minimal fix instead. The C integer promotion
> rules always confuse me (I think otto@ is the only person in the world
> who actually understands them). The way you sprinkled (signed) and
> (unsigned) in the code makes me very suspicious since I don't think it
> does what you think it does. Now using -1 as a magic value for an
> unsigned type isn't entirely uncommon, and the "traditional" way to
> handle this has always been to cast the -1 constant to the appropriate
> type. So...
Yes, the casting was bad and I fixed that in my tree. I just need to add
some more type changes (aml_value!length) and I'll send a new diff.
>
> > Index: dev/acpi/dsdt.c
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/acpi/dsdt.c,v
> > retrieving revision 1.193
> > diff -u -p -r1.193 dsdt.c
> > --- dev/acpi/dsdt.c 15 Mar 2012 18:36:53 -0000 1.193
> > +++ dev/acpi/dsdt.c 30 Mar 2012 22:45:15 -0000
> > @@ -895,7 +892,7 @@ aml_val2int(struct aml_value *rval)
> >
> > /* Sets value into LHS: lhs must already be cleared */
> > struct aml_value *
> > -_aml_setvalue(struct aml_value *lhs, int type, int64_t ival, const void
> > *bval)
> > +_aml_setvalue(struct aml_value *lhs, int type, u_int64_t ival, const void
> > *bval)
> > {
> > memset(&lhs->_, 0x0, sizeof(lhs->_));
> >
> > @@ -923,7 +920,7 @@ _aml_setvalue(struct aml_value *lhs, int
> > memcpy(lhs->v_buffer, bval, ival);
> > break;
> > case AML_OBJTYPE_STRING:
> > - if (ival == -1)
> > + if ((signed)ival == -1)
>
> + if (ival == (u_int64_t)-1)
>
> > ival = strlen((const char *)bval);
> > lhs->length = ival;
> > lhs->v_string = (char *)acpi_os_malloc(ival+1);
> > @@ -1451,11 +1448,11 @@ static char osstring[] = "Macrosift Wind
> > struct aml_defval {
> > const char *name;
> > int type;
> > - int64_t ival;
> > + u_int64_t ival;
> > const void *bval;
> > struct aml_value **gval;
> > } aml_defobj[] = {
> > - { "_OS_", AML_OBJTYPE_STRING, -1, osstring },
> > + { "_OS_", AML_OBJTYPE_STRING, (unsigned)-1, osstring },
>
> and there should be no need to add a cast here.
