Re: [PATCH] ubsan: Introduce CONFIG_UBSAN_FATAL to panic on UBSAN failure

[Julien Grall <julien@xxxxxxx>]

Hi Michal,

On 28/11/2023 18:15, Michal Orzel wrote:
> On 28/11/2023 18:09, Julien Grall wrote:
> > On 28/11/2023 18:00, Michal Orzel wrote:
> > > Hi Julien,
> > >
> > > On 28/11/2023 17:14, Julien Grall wrote:
> > > > Hi Michal,
> > > >
> > > > On 27/11/2023 15:41, Michal Orzel wrote:
> > > > > Introduce the CONFIG_UBSAN_FATAL option to cater to scenarios where prompt
> > > > > attention to undefined behavior issues, notably during CI test runs, is
> > > > > essential. When enabled, this option causes Xen to panic upon detecting
> > > > > UBSAN failure (as the last step in ubsan_epilogue()).
> > > > I have mixed opinions on this patch. This would be a good one if we had
> > > > a Xen mostly free from UBSAN behavior. But this is not the case at least
> > > > on arm32. So we would end up to stop at the first error. IOW, we would
> > > > need to fix the first error before we can see the next one.
> > > Well, this patch introduces a config option disabled by default.
> > I understood this is disabled by default... I am pointing out that I am
> > not convinced about the usefulness until we are at the stage where Xen
> > is normally not reporting any USBAN error.
> >
> > > If we end up enabling it for CI for reasons* stated by Andrew, then the natural 
> > > way
> > > of handling such issues is to do the investigation locally.
> > This will not always be possible. One example is when you are only able
> > to reproduce some of the USBAN errors on a specific HW.
> >
> > > Then, you are not forced
> > > to select this option and you can see all the UBSAN issues if you want.
> > See above, I got that point. I am mostly concerned about the implication
> > in the CI right now.
> >
> > > > So I feel it would be best if the gitlab CI jobs actually check for
> > > > USBAN in the logs and fail if there are any. With that, we can get the
> > > > full list of UBSAN issues on each job.
> > > Well, I prefer Andrew suggestion (both [1] and on IRC), hence this patch.
> > >
> > > *my plan was to first fix the UBSAN issues and then enable this option for CI.
> > That would have been useful to describe your goal after "---". With that
> > in mind, then I suggest to revisit this patch once all the UBSAN issues
> > in a normal build are fixed.
> But this patch does not enable this option for CI automatically, right?
Correct.

> Why are you so keen to push it back?
I have been pushing back because your commit message refers to the CI 
specifically and today this would not really work (read as I would not 
be happy if this option is enabled in the CI right now at least on arm32).
If you want to fail a CI job for UBSAN today, then we need to find a 
different approach so we can get the full list of UBSAN error rather 
than fixing one, retry and then next one.
> Is it because you see no point in this option other than for the upstream CI 
> loop?
Even in the upstream CI loop, I am a little unsure about this approach. 
At least, I feel I would want to see all the reports at once in the CI.
But this is not really a strong feeling.

> I find it useful on a day-to-day
> Xen runs, and I would for sure enable it by default in my config not to miss 
> UBSAN failures.
Fair enough. I view USBAN issues the same a WARN_ON. They all need to be 
investigated. So now you have an inconsistent policy.
Are you are also intending to switch WARN_ON() to fatal? If not, then 
why would UBSAN warnings more important that the other warnings?
Cheers,

--
Julien Grall