Re: [Xen-devel] [PATCH v2 for-4.10] ubsan: add clang 5.0 support

[Julien Grall]

Hi Roger,

On 10/18/2017 08:45 AM, Roger Pau Monne wrote:

clang 5.0 changed the layout of the type_mismatch_data structure and
introduced __ubsan_handle_type_mismatch_v1 and
__ubsan_handle_pointer_overflow.

This commit adds support for the new structure layout, adds the
missing handlers and the new types for type_check_kinds.

Signed-off-by: Roger Pau Monné 
---
Cc: Andrew Cooper 
Cc: George Dunlap 
Cc: Ian Jackson 
Cc: Jan Beulich 
Cc: Konrad Rzeszutek Wilk 
Cc: Stefano Stabellini 
Cc: Tim Deegan 
Cc: Wei Liu 
Cc: Julien Grall 
---
ubsan is an optional feature, not enabled by default and not designed
to be used by production systems. Since this change only touches ubsan
code and it's a bugfix in order for clang to work, I argue it should
be merged into 4.10.


I agree here:

Release-acked-by: Julien Grall 

Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 for-4.10] ubsan: add clang 5.0 support

[Wei Liu]

On Wed, Oct 18, 2017 at 08:45:32AM +0100, Roger Pau Monne wrote:
> clang 5.0 changed the layout of the type_mismatch_data structure and
> introduced __ubsan_handle_type_mismatch_v1 and
> __ubsan_handle_pointer_overflow.
> 
> This commit adds support for the new structure layout, adds the
> missing handlers and the new types for type_check_kinds.
> 
> Signed-off-by: Roger Pau Monné 

With existing comments addressed:

Acked-by: Wei Liu 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 for-4.10] ubsan: add clang 5.0 support

[Roger Pau Monné]

On Wed, Oct 18, 2017 at 03:53:37AM -0600, Jan Beulich wrote:
> >>> On 18.10.17 at 11:42,  wrote:
> > On Wed, Oct 18, 2017 at 03:23:20AM -0600, Jan Beulich wrote:
> >> >>> On 18.10.17 at 09:45,  wrote:
> >> > +void __ubsan_handle_pointer_overflow(struct pointer_overflow_data *data,
> >> > +unsigned long base, unsigned long 
> >> > result)
> >> > +{
> >> > +unsigned long flags;
> >> > +
> >> > +if (suppress_report(>location))
> >> > +return;
> >> > +
> >> > +ubsan_prologue(>location, );
> >> > +
> >> > +if (((long)base >= 0) == ((long)result >= 0))
> >> > +pr_err("pointer operation %s %p to %p\n",
> >> > +base > result ? "underflowed" : "overflowed",
> >> > +(void *)base, (void *)result);
> >> > +else
> >> > +pr_err("pointer index expression with base %p 
> >> > overflowed to %p\n",
> >> > +(void *)base, (void *)result);
> >> 
> >> Would you mind explaining the difference between if and else
> >> branches? (I do realize I should have asked this on v1 already,
> >> but I didn't pay enough attention.) Whatever the idea behind
> >> this, it should probably be explained in a comment, as it looks
> >> to be heuristic.
> > 
> > The upstream commit is:
> > 
> > https://github.com/llvm-mirror/compiler-rt/commit/079b7657767dcc0fb284225c277d
> >  
> > 2b9ce73e423b
> > 
> > However it's lacking a proper commit message. It seems to me like it's
> > there to detect addition of signed + unsigned values when an overflow
> > happens, but I don't really see it's value rather than just using the
> > first message.
> 
> Right - me too. I'd therefore like to simply drop the "if" and the "else"
> branch (likely easily done while committing), and then the change is
> Acked-by: Jan Beulich 

Yes, feel free to drop the if/else and just keep the first error
message.

Thanks, Roger.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 for-4.10] ubsan: add clang 5.0 support

[Jan Beulich]

>>> On 18.10.17 at 11:42,  wrote:
> On Wed, Oct 18, 2017 at 03:23:20AM -0600, Jan Beulich wrote:
>> >>> On 18.10.17 at 09:45,  wrote:
>> > +void __ubsan_handle_pointer_overflow(struct pointer_overflow_data *data,
>> > +  unsigned long base, unsigned long result)
>> > +{
>> > +  unsigned long flags;
>> > +
>> > +  if (suppress_report(>location))
>> > +  return;
>> > +
>> > +  ubsan_prologue(>location, );
>> > +
>> > +  if (((long)base >= 0) == ((long)result >= 0))
>> > +  pr_err("pointer operation %s %p to %p\n",
>> > +  base > result ? "underflowed" : "overflowed",
>> > +  (void *)base, (void *)result);
>> > +  else
>> > +  pr_err("pointer index expression with base %p overflowed to 
>> > %p\n",
>> > +  (void *)base, (void *)result);
>> 
>> Would you mind explaining the difference between if and else
>> branches? (I do realize I should have asked this on v1 already,
>> but I didn't pay enough attention.) Whatever the idea behind
>> this, it should probably be explained in a comment, as it looks
>> to be heuristic.
> 
> The upstream commit is:
> 
> https://github.com/llvm-mirror/compiler-rt/commit/079b7657767dcc0fb284225c277d
>  
> 2b9ce73e423b
> 
> However it's lacking a proper commit message. It seems to me like it's
> there to detect addition of signed + unsigned values when an overflow
> happens, but I don't really see it's value rather than just using the
> first message.

Right - me too. I'd therefore like to simply drop the "if" and the "else"
branch (likely easily done while committing), and then the change is
Acked-by: Jan Beulich 

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 for-4.10] ubsan: add clang 5.0 support

[Roger Pau Monné]

On Wed, Oct 18, 2017 at 03:23:20AM -0600, Jan Beulich wrote:
> >>> On 18.10.17 at 09:45,  wrote:
> > +void __ubsan_handle_pointer_overflow(struct pointer_overflow_data *data,
> > +   unsigned long base, unsigned long result)
> > +{
> > +   unsigned long flags;
> > +
> > +   if (suppress_report(>location))
> > +   return;
> > +
> > +   ubsan_prologue(>location, );
> > +
> > +   if (((long)base >= 0) == ((long)result >= 0))
> > +   pr_err("pointer operation %s %p to %p\n",
> > +   base > result ? "underflowed" : "overflowed",
> > +   (void *)base, (void *)result);
> > +   else
> > +   pr_err("pointer index expression with base %p overflowed to 
> > %p\n",
> > +   (void *)base, (void *)result);
> 
> Would you mind explaining the difference between if and else
> branches? (I do realize I should have asked this on v1 already,
> but I didn't pay enough attention.) Whatever the idea behind
> this, it should probably be explained in a comment, as it looks
> to be heuristic.

The upstream commit is:

https://github.com/llvm-mirror/compiler-rt/commit/079b7657767dcc0fb284225c277d2b9ce73e423b

However it's lacking a proper commit message. It seems to me like it's
there to detect addition of signed + unsigned values when an overflow
happens, but I don't really see it's value rather than just using the
first message.

Thanks, Roger.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 for-4.10] ubsan: add clang 5.0 support

[Jan Beulich]

>>> On 18.10.17 at 09:45,  wrote:
> +void __ubsan_handle_pointer_overflow(struct pointer_overflow_data *data,
> + unsigned long base, unsigned long result)
> +{
> + unsigned long flags;
> +
> + if (suppress_report(>location))
> + return;
> +
> + ubsan_prologue(>location, );
> +
> + if (((long)base >= 0) == ((long)result >= 0))
> + pr_err("pointer operation %s %p to %p\n",
> + base > result ? "underflowed" : "overflowed",
> + (void *)base, (void *)result);
> + else
> + pr_err("pointer index expression with base %p overflowed to 
> %p\n",
> + (void *)base, (void *)result);

Would you mind explaining the difference between if and else
branches? (I do realize I should have asked this on v1 already,
but I didn't pay enough attention.) Whatever the idea behind
this, it should probably be explained in a comment, as it looks
to be heuristic.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel