[Mesa-dev] [PATCH 0/2] Simple Klocwork patches

[Iago Toral]

Hi Juha,

On Sun, 2016-02-07 at 23:37 +0200, Juha-Pekka Heikkilä wrote:
> Hi Iago,
> 
> I know there are lot of places where there is malloc unchecked still
> -- and then there is ralloc which is a story of its own. Reason why I
> think checking these would be remotely useful in windows only (or
> other way around, not under linux kernel) is on Windows one can get
> the null pointer from malloc. On Androids I think memory over
> committing has always been enabled and on Linux I suspect I belong to
> the minority who like to set ulimits for memory.
> 
> I agree checking these mostly is quite useless but there are those
> corners where it may suddenly become valuable. When process is running
> and everything has settled it will be weird if hit any of these checks
> but any code which is run when process is starting I notice is the
> place where things will fail if they fail. This is of course just my
> opinion about the value of these checks but I really dislike
> possibility of segfault when it is coming from a library.
> 
> I didn't quickly notice where _mesa_error() get more heap. Stack it of
> course needs but when I did stress test these _mesa_error() did still
> work. Cannot promise my test was 100% correct though, I think it was
> over year ago when I was playing with it.
> 

I think Matt explained this but I suppose it might still help in some
platforms. Ian's point about using NULL as an offset is also valid.

Anyway, I am not blocking anything, the patches are a small thing and
they might help in some cases so feel to go ahead with them. If you
still need a Reviewed-by for these you can add mine or Samuel's.

Iago

> 
> On Wed, Feb 3, 2016 at 5:12 PM, Iago Toral <itoral at igalia.com> wrote:
> > Hi Juha,
> >
> > I don't know why checking for this might be more relevant in Windows,
> > but in any case:
> >
> > There are a ton of other places in mesa where we allocate memory via
> > calloc/malloc and we don't check that the allocation actually succeeded
> > so I am not sure that fixing a couple of instances of *small*
> > allocations changes anything.
> >
> > IMHO, this kind of things are only really useful when allocating memory
> > for large amounts of data, otherwise even if you check for a NULL
> > allocation you still need to make sure that you don't need any extra
> > memory to handle that situation, and _mesa_error() needs memory, so it
> > is probably not really giving us anything in practice other than
> > silencing Klocwork...
> >
> > Iago
> >
> > On Wed, 2016-02-03 at 10:56 +0200, Juha-Pekka Heikkila wrote:
> >> I'm thinking these things maybe could be wrapped up inside something like
> >> "#ifdef windows" or so in the future. At least for Android and Linux these
> >> are normally quite useless.
> >>
> >> /Juha-Pekka
> >>
> >> Juha-Pekka Heikkila (2):
> >>   i965: in brw_link_shader() react to low memory
> >>   glsl: Check for null pointer at ir_variable_refcount_visitor()
> >>
> >>  src/compiler/glsl/ir_variable_refcount.cpp | 7 +++++++
> >>  src/mesa/drivers/dri/i965/brw_link.cpp     | 4 ++++
> >>  src/mesa/main/ff_fragment_shader.cpp       | 6 ++++--
> >>  3 files changed, 15 insertions(+), 2 deletions(-)
> >>
> >
> >
>