All of lore.kernel.org
 help / color / mirror / Atom feed
* denc nullptr issue
@ 2017-03-07 22:48 Sage Weil
  2017-03-07 23:54 ` Willem Jan Withagen
  2017-03-08 17:22 ` Adam C. Emerson
  0 siblings, 2 replies; 9+ messages in thread
From: Sage Weil @ 2017-03-07 22:48 UTC (permalink / raw)
  To: ceph-devel

We have three competing PRs to address the crashes when -O0 is used:

lixiaoy1	https://github.com/ceph/ceph/pull/12796
kefu		https://github.com/ceph/ceph/pull/13819
adam		https://github.com/ceph/ceph/pull/12931

Adam's is appealing because it will ensure the bounded encodes are in fact 
constant at compile time.  Unfortunately, it requires you to define DENC 
twice, like so:

-  DENC_FEATURED(bar_t, v, p, f) {
+  DENC_FEATURED_BOUNDED(bar_t, v, p, f) {
     ::denc(v.a, p, f);
     ::denc(v.b, p, f);
   }
+  DENC_FEATURED_BOUND_ENCODE(bar_t, p, f) {
+    DENC_START(1, 1, p);
+    ::denc(bounded_t<decltype(bar_t::a)>{}, p, f);
+    ::denc(bounded_t<decltype(bar_t::b)>{}, p, f);
+    DENC_FINISH(p);
+  }

which feels like a deal-killer to me... I think I'd rather just drop the 
check entirely and rely on the developer to be careful.

The other two PRs fix up the handful of type(s) that were actually causing 
failures to manually define bound (again making the single DENC macro 
unusable).

What about instead changing all of the instances of

      denc(*(const T*)nullptr, elem_size);

with a macro that will either do the above "*(const T*)nullptr" -or-, if 
it looks like this is a debug build or compiler optimizations are off or 
some other compiler environment indicates it won't work, replace it with 
"T()"?  That avoids crashes in the -O0 case, but still lets us keep some 
safeguard for other builds?

Any other ideas?

sage


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: denc nullptr issue
  2017-03-07 22:48 denc nullptr issue Sage Weil
@ 2017-03-07 23:54 ` Willem Jan Withagen
  2017-03-08  3:46   ` Brad Hubbard
  2017-03-08 17:22 ` Adam C. Emerson
  1 sibling, 1 reply; 9+ messages in thread
From: Willem Jan Withagen @ 2017-03-07 23:54 UTC (permalink / raw)
  To: Sage Weil, ceph-devel

On 7-3-2017 23:48, Sage Weil wrote:
> We have three competing PRs to address the crashes when -O0 is used:
> 
> lixiaoy1	https://github.com/ceph/ceph/pull/12796
> kefu		https://github.com/ceph/ceph/pull/13819
> adam		https://github.com/ceph/ceph/pull/12931
> 
> Adam's is appealing because it will ensure the bounded encodes are in fact 
> constant at compile time.  Unfortunately, it requires you to define DENC 
> twice, like so:
> 
> -  DENC_FEATURED(bar_t, v, p, f) {
> +  DENC_FEATURED_BOUNDED(bar_t, v, p, f) {
>      ::denc(v.a, p, f);
>      ::denc(v.b, p, f);
>    }
> +  DENC_FEATURED_BOUND_ENCODE(bar_t, p, f) {
> +    DENC_START(1, 1, p);
> +    ::denc(bounded_t<decltype(bar_t::a)>{}, p, f);
> +    ::denc(bounded_t<decltype(bar_t::b)>{}, p, f);
> +    DENC_FINISH(p);
> +  }
> 
> which feels like a deal-killer to me... I think I'd rather just drop the 
> check entirely and rely on the developer to be careful.
> 
> The other two PRs fix up the handful of type(s) that were actually causing 
> failures to manually define bound (again making the single DENC macro 
> unusable).
> 
> What about instead changing all of the instances of
> 
>       denc(*(const T*)nullptr, elem_size);
> 
> with a macro that will either do the above "*(const T*)nullptr" -or-, if 
> it looks like this is a debug build or compiler optimizations are off or 
> some other compiler environment indicates it won't work, replace it with 
> "T()"?  That avoids crashes in the -O0 case, but still lets us keep some 
> safeguard for other builds?
> 
> Any other ideas?

Hi Sage,

Clang is already complaining about the current construction, and any fix
would be nice to not have Clang complain.
I have not yet run into crashes (even when compiling with -O0), but that
might be because the current crash is in Bluestore I think. And I do not
build that for FreeBSD.

Why would 2 defines be a deal breaker, if otherwise the burden also is
the the developer that needs to take care of possible pitfalls.

And I would expect to have your second option to still run into warnings
with Clang if not compiling with -O0 and we still compile "*(const
T*)nullptr"

--WjW


/home/jenkins/workspace/ceph-master/src/include/denc.h:1162:10: warning:
binding dereferenced null pointer to reference has undefined behavior
[-Wnull-dereference]
    denc(*(bool *)nullptr, p);
         ^~~~~~~~~~~~~~~~
/home/jenkins/workspace/ceph-master/src/include/denc.h:1169:10: warning:
binding dereferenced null pointer to reference has undefined behavior
[-Wnull-dereference]
    denc(*(bool *)nullptr, p);
         ^~~~~~~~~~~~~~~~
/home/jenkins/workspace/ceph-master/src/include/denc.h:1236:10: warning:
binding dereferenced null pointer to reference has undefined behavior
[-Wnull-dereference]
    denc(*(bool *)nullptr, p);
         ^~~~~~~~~~~~~~~~
/home/jenkins/workspace/ceph-master/src/include/denc.h:682:12: warning:
binding dereferenced null pointer to reference has undefined behavior
[-Wnull-dereference]
      denc(*(const T*)nullptr, elem_size);
           ^~~~~~~~~~~~~~~~~~
/home/jenkins/workspace/ceph-master/src/include/denc.h:1304:11: note: in
instantiation of function template specialization
'_denc::container_base<std::vector,
_denc::pushback_details<std::__1::vector<snapid_t,
std::__1::allocator<snapid_t> > >, snapid_t,
std::__1::allocator<snapid_t> >::bound_encode<snapid_t>' requested here

  traits::bound_encode(o, len);
          ^
/home/jenkins/workspace/ceph-master/src/common/snap_types.h:58:7: note:
in instantiation of function template specialization
'encode<std::__1::vector<snapid_t, std::__1::allocator<snapid_t> >,
denc_traits<std::__1::vector<snapid_t, std::__1::allocator<snapid_t> >,
void> >' requested here
    ::encode(snaps, bl);
      ^



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: denc nullptr issue
  2017-03-07 23:54 ` Willem Jan Withagen
@ 2017-03-08  3:46   ` Brad Hubbard
  0 siblings, 0 replies; 9+ messages in thread
From: Brad Hubbard @ 2017-03-08  3:46 UTC (permalink / raw)
  To: Willem Jan Withagen; +Cc: Sage Weil, ceph-devel

Indeed, there are around 16 issues clang picks up with denc.h (note
that some may be false positives).

http://people.redhat.com/bhubbard/scan-build.17-03-02/index.html

On Wed, Mar 8, 2017 at 9:54 AM, Willem Jan Withagen <wjw@digiware.nl> wrote:
> On 7-3-2017 23:48, Sage Weil wrote:
>> We have three competing PRs to address the crashes when -O0 is used:
>>
>> lixiaoy1      https://github.com/ceph/ceph/pull/12796
>> kefu          https://github.com/ceph/ceph/pull/13819
>> adam          https://github.com/ceph/ceph/pull/12931
>>
>> Adam's is appealing because it will ensure the bounded encodes are in fact
>> constant at compile time.  Unfortunately, it requires you to define DENC
>> twice, like so:
>>
>> -  DENC_FEATURED(bar_t, v, p, f) {
>> +  DENC_FEATURED_BOUNDED(bar_t, v, p, f) {
>>      ::denc(v.a, p, f);
>>      ::denc(v.b, p, f);
>>    }
>> +  DENC_FEATURED_BOUND_ENCODE(bar_t, p, f) {
>> +    DENC_START(1, 1, p);
>> +    ::denc(bounded_t<decltype(bar_t::a)>{}, p, f);
>> +    ::denc(bounded_t<decltype(bar_t::b)>{}, p, f);
>> +    DENC_FINISH(p);
>> +  }
>>
>> which feels like a deal-killer to me... I think I'd rather just drop the
>> check entirely and rely on the developer to be careful.
>>
>> The other two PRs fix up the handful of type(s) that were actually causing
>> failures to manually define bound (again making the single DENC macro
>> unusable).
>>
>> What about instead changing all of the instances of
>>
>>       denc(*(const T*)nullptr, elem_size);
>>
>> with a macro that will either do the above "*(const T*)nullptr" -or-, if
>> it looks like this is a debug build or compiler optimizations are off or
>> some other compiler environment indicates it won't work, replace it with
>> "T()"?  That avoids crashes in the -O0 case, but still lets us keep some
>> safeguard for other builds?
>>
>> Any other ideas?
>
> Hi Sage,
>
> Clang is already complaining about the current construction, and any fix
> would be nice to not have Clang complain.
> I have not yet run into crashes (even when compiling with -O0), but that
> might be because the current crash is in Bluestore I think. And I do not
> build that for FreeBSD.
>
> Why would 2 defines be a deal breaker, if otherwise the burden also is
> the the developer that needs to take care of possible pitfalls.
>
> And I would expect to have your second option to still run into warnings
> with Clang if not compiling with -O0 and we still compile "*(const
> T*)nullptr"
>
> --WjW
>
>
> /home/jenkins/workspace/ceph-master/src/include/denc.h:1162:10: warning:
> binding dereferenced null pointer to reference has undefined behavior
> [-Wnull-dereference]
>     denc(*(bool *)nullptr, p);
>          ^~~~~~~~~~~~~~~~
> /home/jenkins/workspace/ceph-master/src/include/denc.h:1169:10: warning:
> binding dereferenced null pointer to reference has undefined behavior
> [-Wnull-dereference]
>     denc(*(bool *)nullptr, p);
>          ^~~~~~~~~~~~~~~~
> /home/jenkins/workspace/ceph-master/src/include/denc.h:1236:10: warning:
> binding dereferenced null pointer to reference has undefined behavior
> [-Wnull-dereference]
>     denc(*(bool *)nullptr, p);
>          ^~~~~~~~~~~~~~~~
> /home/jenkins/workspace/ceph-master/src/include/denc.h:682:12: warning:
> binding dereferenced null pointer to reference has undefined behavior
> [-Wnull-dereference]
>       denc(*(const T*)nullptr, elem_size);
>            ^~~~~~~~~~~~~~~~~~
> /home/jenkins/workspace/ceph-master/src/include/denc.h:1304:11: note: in
> instantiation of function template specialization
> '_denc::container_base<std::vector,
> _denc::pushback_details<std::__1::vector<snapid_t,
> std::__1::allocator<snapid_t> > >, snapid_t,
> std::__1::allocator<snapid_t> >::bound_encode<snapid_t>' requested here
>
>   traits::bound_encode(o, len);
>           ^
> /home/jenkins/workspace/ceph-master/src/common/snap_types.h:58:7: note:
> in instantiation of function template specialization
> 'encode<std::__1::vector<snapid_t, std::__1::allocator<snapid_t> >,
> denc_traits<std::__1::vector<snapid_t, std::__1::allocator<snapid_t> >,
> void> >' requested here
>     ::encode(snaps, bl);
>       ^
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Cheers,
Brad

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: denc nullptr issue
  2017-03-07 22:48 denc nullptr issue Sage Weil
  2017-03-07 23:54 ` Willem Jan Withagen
@ 2017-03-08 17:22 ` Adam C. Emerson
  2017-03-08 17:28   ` Sage Weil
  1 sibling, 1 reply; 9+ messages in thread
From: Adam C. Emerson @ 2017-03-08 17:22 UTC (permalink / raw)
  To: The Sacred Order of the Squid Cybernetic

On 07/03/2017, Sage Weil wrote:
> We have three competing PRs to address the crashes when -O0 is used:
> 
> lixiaoy1	https://github.com/ceph/ceph/pull/12796
> kefu		https://github.com/ceph/ceph/pull/13819
> adam		https://github.com/ceph/ceph/pull/12931
> 
> Adam's is appealing because it will ensure the bounded encodes are in fact 
> constant at compile time.  Unfortunately, it requires you to define DENC 
> twice, like so:
> 
> -  DENC_FEATURED(bar_t, v, p, f) {
> +  DENC_FEATURED_BOUNDED(bar_t, v, p, f) {
>      ::denc(v.a, p, f);
>      ::denc(v.b, p, f);
>    }
> +  DENC_FEATURED_BOUND_ENCODE(bar_t, p, f) {
> +    DENC_START(1, 1, p);
> +    ::denc(bounded_t<decltype(bar_t::a)>{}, p, f);
> +    ::denc(bounded_t<decltype(bar_t::b)>{}, p, f);
> +    DENC_FINISH(p);
> +  }
> 
> which feels like a deal-killer to me... I think I'd rather just drop the 
> check entirely and rely on the developer to be careful.

I have an fix for that in mind (it's why I haven't been pushing that
PR too hard) but it's complicated.

Essentially, for bounded types, I /think/ I can hack together a way
let people do something like:

struct Foo {
  TypeA a;
  TypeB b;
  TypeC c;
};

BUILD_BOUNDED_DENC(Foo, &Foo::a, &Foo::b, &Foo::c);

and not have to divide things up. We could probably go things
involving fancier pants to handle features and other stuff.

> The other two PRs fix up the handful of type(s) that were actually
> causing failures to manually define bound (again making the single
> DENC macro unusable).
> 
> What about instead changing all of the instances of
> 
>       denc(*(const T*)nullptr, elem_size);
> 
> with a macro that will either do the above "*(const T*)nullptr" -or-, if 
> it looks like this is a debug build or compiler optimizations are off or 
> some other compiler environment indicates it won't work, replace it with 
> "T()"?  That avoids crashes in the -O0 case, but still lets us keep some 
> safeguard for other builds?

I would really, really rather not do this. The whole nullptr trick is
definitely one of the black rituals of Nasal Goetia. Even if
redefining it happens to make -O0 on gcc work now, other compilers,
GCC on other architectures, or other versions of GCC are free to start
crashing again.

-- 
Senior Software Engineer           Red Hat Storage, Ann Arbor, MI, US
IRC: Aemerson@{RedHat, OFTC}
0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C  7C12 80F7 544B 90ED BFB9

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: denc nullptr issue
  2017-03-08 17:22 ` Adam C. Emerson
@ 2017-03-08 17:28   ` Sage Weil
  2017-03-08 17:44     ` Adam C. Emerson
  0 siblings, 1 reply; 9+ messages in thread
From: Sage Weil @ 2017-03-08 17:28 UTC (permalink / raw)
  To: Adam C. Emerson; +Cc: The Sacred Order of the Squid Cybernetic

On Wed, 8 Mar 2017, Adam C. Emerson wrote:
> On 07/03/2017, Sage Weil wrote:
> > We have three competing PRs to address the crashes when -O0 is used:
> > 
> > lixiaoy1	https://github.com/ceph/ceph/pull/12796
> > kefu		https://github.com/ceph/ceph/pull/13819
> > adam		https://github.com/ceph/ceph/pull/12931
> > 
> > Adam's is appealing because it will ensure the bounded encodes are in fact 
> > constant at compile time.  Unfortunately, it requires you to define DENC 
> > twice, like so:
> > 
> > -  DENC_FEATURED(bar_t, v, p, f) {
> > +  DENC_FEATURED_BOUNDED(bar_t, v, p, f) {
> >      ::denc(v.a, p, f);
> >      ::denc(v.b, p, f);
> >    }
> > +  DENC_FEATURED_BOUND_ENCODE(bar_t, p, f) {
> > +    DENC_START(1, 1, p);
> > +    ::denc(bounded_t<decltype(bar_t::a)>{}, p, f);
> > +    ::denc(bounded_t<decltype(bar_t::b)>{}, p, f);
> > +    DENC_FINISH(p);
> > +  }
> > 
> > which feels like a deal-killer to me... I think I'd rather just drop the 
> > check entirely and rely on the developer to be careful.
> 
> I have an fix for that in mind (it's why I haven't been pushing that
> PR too hard) but it's complicated.
> 
> Essentially, for bounded types, I /think/ I can hack together a way
> let people do something like:
> 
> struct Foo {
>   TypeA a;
>   TypeB b;
>   TypeC c;
> };
> 
> BUILD_BOUNDED_DENC(Foo, &Foo::a, &Foo::b, &Foo::c);
> 
> and not have to divide things up. We could probably go things
> involving fancier pants to handle features and other stuff.

That removes a lot of the flexibily of the current DENC.  OTOH, it might 
be that the set of cases where the above is insufficient and the current 
DENC still is is pretty small?
 
> > The other two PRs fix up the handful of type(s) that were actually
> > causing failures to manually define bound (again making the single
> > DENC macro unusable).
> > 
> > What about instead changing all of the instances of
> > 
> >       denc(*(const T*)nullptr, elem_size);
> > 
> > with a macro that will either do the above "*(const T*)nullptr" -or-, if 
> > it looks like this is a debug build or compiler optimizations are off or 
> > some other compiler environment indicates it won't work, replace it with 
> > "T()"?  That avoids crashes in the -O0 case, but still lets us keep some 
> > safeguard for other builds?
> 
> I would really, really rather not do this. The whole nullptr trick is
> definitely one of the black rituals of Nasal Goetia. Even if
> redefining it happens to make -O0 on gcc work now, other compilers,
> GCC on other architectures, or other versions of GCC are free to start
> crashing again.

I'm inclined to just drop the trick and pass T() everywhere instead... at 
least until we have something better.  Objections?

sage

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: denc nullptr issue
  2017-03-08 17:28   ` Sage Weil
@ 2017-03-08 17:44     ` Adam C. Emerson
  2017-03-08 17:52       ` Gregory Farnum
  2017-03-08 18:23       ` Sage Weil
  0 siblings, 2 replies; 9+ messages in thread
From: Adam C. Emerson @ 2017-03-08 17:44 UTC (permalink / raw)
  To: The Sacred Order of the Squid Cybernetic

On 08/03/2017, Sage Weil wrote:
> That removes a lot of the flexibily of the current DENC.  OTOH, it
> might be that the set of cases where the above is insufficient and
> the current DENC still is is pretty small?

Well! In the bounded case we know there's a limit on the amount of
flexibility we /can/ have, just because we're not allowed to vary the
encoding based on anything BUT supplied feature bits, so I don't think
we have to worry about encodings.

I /suspect/ the amount of flexibility we'd end up using for bounded
objects if fairly small, though I can, as mentioned, think of fancier
things to do. (I think any interesting, likely cases could be handled
by const functions that take some set of class members and return some
stuff to be encoded. Which coudl be done in the same sort of
semideclarative style.)

> I'm inclined to just drop the trick and pass T() everywhere
> instead... at least until we have something better.  Objections?

Two potential ones. On the one hand that would require everything
dencable to be default constructible. That isn't a problem now but
it's something I'd like to not have be the case forever. (But that
bridge can be crossable in the Idealized Future.)

The more pressing objection might be that if the current idea is to
make sure bounded objects really /are/ bounded, passing T() might be
the worst of all worlds. If caught at compile time the problem goes
away. If caught at runtime with the nullptr it at least gives a
definite error in a useful place. If we pass T() then an object that
claims to be bounded but /isn't/ could calculate the wrong bound and
then we'd have to debug overruns or underruns or something.

Given that, I think if we're going to give up on this feature for now,
the 'least bad' alternative would be to just document the requirements
for an object that claims to be bounded but pass the supplied object
reference in all cases. We might have poorer performance from someone
violating the guarantee, but not outright incorrectness. Also errors
of this sort seem as if they should be easily eyeballed. One need only
look for objects that claim to be bounded and see how they implement
their bound_encode method.

-- 
Senior Software Engineer           Red Hat Storage, Ann Arbor, MI, US
IRC: Aemerson@{RedHat, OFTC}
0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C  7C12 80F7 544B 90ED BFB9

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: denc nullptr issue
  2017-03-08 17:44     ` Adam C. Emerson
@ 2017-03-08 17:52       ` Gregory Farnum
  2017-03-08 18:04         ` Adam C. Emerson
  2017-03-08 18:23       ` Sage Weil
  1 sibling, 1 reply; 9+ messages in thread
From: Gregory Farnum @ 2017-03-08 17:52 UTC (permalink / raw)
  To: The Sacred Order of the Squid Cybernetic

On Wed, Mar 8, 2017 at 9:44 AM, Adam C. Emerson <aemerson@redhat.com> wrote:
> On 08/03/2017, Sage Weil wrote:
>> That removes a lot of the flexibily of the current DENC.  OTOH, it
>> might be that the set of cases where the above is insufficient and
>> the current DENC still is is pretty small?
>
> Well! In the bounded case we know there's a limit on the amount of
> flexibility we /can/ have, just because we're not allowed to vary the
> encoding based on anything BUT supplied feature bits, so I don't think
> we have to worry about encodings.
>
> I /suspect/ the amount of flexibility we'd end up using for bounded
> objects if fairly small, though I can, as mentioned, think of fancier
> things to do. (I think any interesting, likely cases could be handled
> by const functions that take some set of class members and return some
> stuff to be encoded. Which coudl be done in the same sort of
> semideclarative style.)

I haven't followed this discussion closely, but when I saw your macro
usage the thing that struck me was the lack of versioning, which is
what I assume Sage is referring to. If there's a good way to define
versioned encoding that would probably be fine?
Unless the nullptr issue strikes more complicated structures — we need
to hand-write the OSDMap encoder for instance, and clever macros will
never be clever enough for that.
-Greg

>
>> I'm inclined to just drop the trick and pass T() everywhere
>> instead... at least until we have something better.  Objections?
>
> Two potential ones. On the one hand that would require everything
> dencable to be default constructible. That isn't a problem now but
> it's something I'd like to not have be the case forever. (But that
> bridge can be crossable in the Idealized Future.)
>
> The more pressing objection might be that if the current idea is to
> make sure bounded objects really /are/ bounded, passing T() might be
> the worst of all worlds. If caught at compile time the problem goes
> away. If caught at runtime with the nullptr it at least gives a
> definite error in a useful place. If we pass T() then an object that
> claims to be bounded but /isn't/ could calculate the wrong bound and
> then we'd have to debug overruns or underruns or something.
>
> Given that, I think if we're going to give up on this feature for now,
> the 'least bad' alternative would be to just document the requirements
> for an object that claims to be bounded but pass the supplied object
> reference in all cases. We might have poorer performance from someone
> violating the guarantee, but not outright incorrectness. Also errors
> of this sort seem as if they should be easily eyeballed. One need only
> look for objects that claim to be bounded and see how they implement
> their bound_encode method.
>
> --
> Senior Software Engineer           Red Hat Storage, Ann Arbor, MI, US
> IRC: Aemerson@{RedHat, OFTC}
> 0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C  7C12 80F7 544B 90ED BFB9
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: denc nullptr issue
  2017-03-08 17:52       ` Gregory Farnum
@ 2017-03-08 18:04         ` Adam C. Emerson
  0 siblings, 0 replies; 9+ messages in thread
From: Adam C. Emerson @ 2017-03-08 18:04 UTC (permalink / raw)
  To: The Sacred Order of the Squid Cybernetic

On 08/03/2017, Gregory Farnum wrote:
> I haven't followed this discussion closely, but when I saw your macro
> usage the thing that struck me was the lack of versioning, which is
> what I assume Sage is referring to. If there's a good way to define
> versioned encoding that would probably be fine?
> Unless the nullptr issue strikes more complicated structures — we need
> to hand-write the OSDMap encoder for instance, and clever macros will
> never be clever enough for that.
> -Greg

At least as far as I'm aware the only way we could output different
versions would be in response to feature bits, since bounded objects
are only allowed to depend on the type and the set of feature bits for
the size.

I think doing something based on feature bits is doable, though it
might be slightly baroque and tempt a combinatorial explosion for
something that is bounded and features and varies quite a bit with a
lot of bits.

To be honest while this might be fun to write, it's probably more of a
compile-time-DSL than the value of the check warrants, so I think I'd
advocate just taking 'bounded' as an annotation by the author of a
class and leaving it up to him to make sure his class actually is
bounded.

> >
> >> I'm inclined to just drop the trick and pass T() everywhere
> >> instead... at least until we have something better.  Objections?
> >
> > Two potential ones. On the one hand that would require everything
> > dencable to be default constructible. That isn't a problem now but
> > it's something I'd like to not have be the case forever. (But that
> > bridge can be crossable in the Idealized Future.)
> >
> > The more pressing objection might be that if the current idea is to
> > make sure bounded objects really /are/ bounded, passing T() might be
> > the worst of all worlds. If caught at compile time the problem goes
> > away. If caught at runtime with the nullptr it at least gives a
> > definite error in a useful place. If we pass T() then an object that
> > claims to be bounded but /isn't/ could calculate the wrong bound and
> > then we'd have to debug overruns or underruns or something.
> >
> > Given that, I think if we're going to give up on this feature for now,
> > the 'least bad' alternative would be to just document the requirements
> > for an object that claims to be bounded but pass the supplied object
> > reference in all cases. We might have poorer performance from someone
> > violating the guarantee, but not outright incorrectness. Also errors
> > of this sort seem as if they should be easily eyeballed. One need only
> > look for objects that claim to be bounded and see how they implement
> > their bound_encode method.
> >
> > --
> > Senior Software Engineer           Red Hat Storage, Ann Arbor, MI, US
> > IRC: Aemerson@{RedHat, OFTC}
> > 0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C  7C12 80F7 544B 90ED BFB9
> > --
> > To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Senior Software Engineer           Red Hat Storage, Ann Arbor, MI, US
IRC: Aemerson@{RedHat, OFTC}
0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C  7C12 80F7 544B 90ED BFB9

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: denc nullptr issue
  2017-03-08 17:44     ` Adam C. Emerson
  2017-03-08 17:52       ` Gregory Farnum
@ 2017-03-08 18:23       ` Sage Weil
  1 sibling, 0 replies; 9+ messages in thread
From: Sage Weil @ 2017-03-08 18:23 UTC (permalink / raw)
  To: Adam C. Emerson; +Cc: The Sacred Order of the Squid Cybernetic

On Wed, 8 Mar 2017, Adam C. Emerson wrote:
> On 08/03/2017, Sage Weil wrote:
> > That removes a lot of the flexibily of the current DENC.  OTOH, it
> > might be that the set of cases where the above is insufficient and
> > the current DENC still is is pretty small?
> 
> Well! In the bounded case we know there's a limit on the amount of
> flexibility we /can/ have, just because we're not allowed to vary the
> encoding based on anything BUT supplied feature bits, so I don't think
> we have to worry about encodings.
> 
> I /suspect/ the amount of flexibility we'd end up using for bounded
> objects if fairly small, though I can, as mentioned, think of fancier
> things to do. (I think any interesting, likely cases could be handled
> by const functions that take some set of class members and return some
> stuff to be encoded. Which coudl be done in the same sort of
> semideclarative style.)

Thinking this through a bit more, I think the most common case is that you 
have a method with struct_v and compat_v values and some items exist only 
in later encodings.  That isn't easily captured in a macro like the 
above...

> > I'm inclined to just drop the trick and pass T() everywhere
> > instead... at least until we have something better.  Objections?
> 
> Two potential ones. On the one hand that would require everything
> dencable to be default constructible. That isn't a problem now but
> it's something I'd like to not have be the case forever. (But that
> bridge can be crossable in the Idealized Future.)
> 
> The more pressing objection might be that if the current idea is to
> make sure bounded objects really /are/ bounded, passing T() might be
> the worst of all worlds. If caught at compile time the problem goes
> away. If caught at runtime with the nullptr it at least gives a
> definite error in a useful place. If we pass T() then an object that
> claims to be bounded but /isn't/ could calculate the wrong bound and
> then we'd have to debug overruns or underruns or something.
> 
> Given that, I think if we're going to give up on this feature for now,
> the 'least bad' alternative would be to just document the requirements
> for an object that claims to be bounded but pass the supplied object
> reference in all cases. We might have poorer performance from someone
> violating the guarantee, but not outright incorrectness. Also errors
> of this sort seem as if they should be easily eyeballed. One need only
> look for objects that claim to be bounded and see how they implement
> their bound_encode method.

Oh, right.  Yeah, let's do that.  I guess we just have to be a bit careful 
with the containers so that they call denc on the first item only if it is 
non-empty() (vs blindly multiplying elem_size by size() like they do now).

sage

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2017-03-08 18:25 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-07 22:48 denc nullptr issue Sage Weil
2017-03-07 23:54 ` Willem Jan Withagen
2017-03-08  3:46   ` Brad Hubbard
2017-03-08 17:22 ` Adam C. Emerson
2017-03-08 17:28   ` Sage Weil
2017-03-08 17:44     ` Adam C. Emerson
2017-03-08 17:52       ` Gregory Farnum
2017-03-08 18:04         ` Adam C. Emerson
2017-03-08 18:23       ` Sage Weil

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.