[PATCH 3/3] sandbox: work around missing of_add_memory_bank()

[PATCH 3/3] sandbox: work around missing of_add_memory_bank()

[Holger Schurig]

From: Holger Schurig <holgerschurig@gmail.com>

The goal of "make ARCH=sandbox allyesconfig && make all" is not to
generate a sensible barebox that you'd use. The goal is to create
as much code coverage as possible, so that you see compiler warnings
are can send barebox throught a static checker.

Therefore this simple band-aid to compile drivers/of/base.c wouldn't
create a working device tree implementation, but it will compile.

Signed-off-by: Holger Schurig <holgerschurig@gmail.com>
---
 drivers/of/base.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index c440a69..69d9b09 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1713,8 +1713,10 @@ int of_add_memory(struct device_node *node, bool dump)
 			continue;
 		}
 
+#ifndef CONFIG_SANDBOX
 		of_add_memory_bank(node, dump, n,
 				res.start, resource_size(&res));
+#endif
 		n++;
 	}
 
-- 
1.8.5.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] sandbox: work around missing of_add_memory_bank()

[Lucas Stach]

Am Dienstag, den 22.07.2014, 09:57 +0200 schrieb Holger Schurig:
> From: Holger Schurig <holgerschurig@gmail.com>
> 
> The goal of "make ARCH=sandbox allyesconfig && make all" is not to
> generate a sensible barebox that you'd use. The goal is to create
> as much code coverage as possible, so that you see compiler warnings
> are can send barebox throught a static checker.
> 
> Therefore this simple band-aid to compile drivers/of/base.c wouldn't
> create a working device tree implementation, but it will compile.
> 
> Signed-off-by: Holger Schurig <holgerschurig@gmail.com>
> ---
>  drivers/of/base.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index c440a69..69d9b09 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1713,8 +1713,10 @@ int of_add_memory(struct device_node *node, bool dump)
>  			continue;
>  		}
>  
> +#ifndef CONFIG_SANDBOX
>  		of_add_memory_bank(node, dump, n,
>  				res.start, resource_size(&res));
> +#endif
>  		n++;
>  	}
>  
I really dislike this patch. This adds ifdeffery (which everyone hates)
just for the sake of a static checker that depends on the build to be
run. There are a lot of static checkers out there which don't have this
requirement.

If you still want the build to include this stuff, fix it by adding a
working of_add_memory_bank() for sandbox, this would be both useful in a
general sense and fix your problem.

Regards,
Lucas

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] sandbox: work around missing of_add_memory_bank()

[Holger Schurig]

There are no memory banks in the sandbox, so adding a working
of_add_memory_bank?  How should that work? Or do you mean adding a
dummy of_add_memory_bank() to arch/sandbox/os/board.c?

> his adds ifdeffery (which everyone hates) just for the sake of a static checker

You probably never have run "scan-build make" after "apt-get install
clang-3.5" ?!?!   For me, this two lines are minimal, and the amount
of warnings that scan-build finds in barebox is staggering.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] sandbox: work around missing of_add_memory_bank()

[Lucas Stach]

Am Dienstag, den 22.07.2014, 10:39 +0200 schrieb Holger Schurig:
> There are no memory banks in the sandbox, so adding a working
> of_add_memory_bank?  How should that work? Or do you mean adding a
> dummy of_add_memory_bank() to arch/sandbox/os/board.c?
> 
Can you please elaborate why of_add_memory() doesn't work if no memory
banks are there? Why do you need this ifdef in the first place? Your
commit message fails to explain this.

> > his adds ifdeffery (which everyone hates) just for the sake of a static checker
> 
> You probably never have run "scan-build make" after "apt-get install
> clang-3.5" ?!?!   For me, this two lines are minimal, and the amount
> of warnings that scan-build finds in barebox is staggering.

This argument doesn't work. The more interesting point here would be to
know how much of those are real problems. Different static checkers
expose vastly different signal-to-noise ratios.

I have run barebox through different static checkers, but as a fact I
default to only use those which don't require a build to be run and have
a high signal-to-noise ratio. Which doesn't mean clangs scan-build
couldn't be a welcome addition, so please convince me.

Regards,
Lucas
-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] sandbox: work around missing of_add_memory_bank()

[Holger Schurig]

I don't convince you. Eat your own dog foot then. Ignore my patch.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] sandbox: work around missing of_add_memory_bank()

[Holger Schurig]

Okay, another post, with less heat.

I asked you specifically if a proposed solution would be ok. You
didn't answer at all. That proposed solution would still not "work"
(it won't add a memory bank, because AFAIK in sandbox there are no
memory banks at all, it just uses the hosts memory). It might compile,
however and it might be a bit of unneeded code in the "make
ARCH=sandbox sandbox_defconfig && make all" case.

The tone of your mail made me think that I actually cannot convince
you, that you don't want this. Your reference to signal-to-noise made
me think this. I got the impression that you're dismissing the concept
of static checking and of code-massaging to make that easier.

I don't need to contribute to barebox. My barebox is running for my
board, I can stop now. I don't need to barebox to promote myself, e.g.
as a freelancer. Yet I also don't want to contribute crap to barebox.
So if my attempts are not going any further, and specific question if
XYZ would be ok and instead I get junk about signal-to-noise back,
then I simply aren't inclined to promote things further. Feel yourself
now utterly unconvinced :-)

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] sandbox: work around missing of_add_memory_bank()

[Lucas Stach]

Am Dienstag, den 22.07.2014, 11:09 +0200 schrieb Holger Schurig:
> Okay, another post, with less heat.
> 
Yes, please let us keep the heat out of this argument.

> I asked you specifically if a proposed solution would be ok. You
> didn't answer at all. That proposed solution would still not "work"
> (it won't add a memory bank, because AFAIK in sandbox there are no
> memory banks at all, it just uses the hosts memory). It might compile,
> however and it might be a bit of unneeded code in the "make
> ARCH=sandbox sandbox_defconfig && make all" case.
> 
I wasn't able to give any specific advice as I admitted I did not
understand the problem yet. I'm aware that there are no memory banks in
sandbox, but this doesn't explain a build failure. Your commit message
unfortunately didn't explain this either, that's why I asked you to
elaborate.

Now I actually looked up the code and I think an easier solution would
be to allow CONFIG_OFTREE_MEM_GENERIC to be enabled on sandbox. Would
this work for you?

> The tone of your mail made me think that I actually cannot convince
> you, that you don't want this. Your reference to signal-to-noise made
> me think this. I got the impression that you're dismissing the concept
> of static checking and of code-massaging to make that easier.
> 

Sorry, if it seemed like I wanted to trash your contribution. This
wasn't my intention at all. I'm absolutely in favor of static checking
but found that clangs scan-build gives at lot of false positives on
other projects and I'm not really keen on adding ifdefs just for this
use-case. But as I said above there may be another solution.

Regards,
Lucas

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] sandbox: work around missing of_add_memory_bank()

[Holger Schurig]

> but this doesn't explain a build failure

The build failure was

  LD      barebox
drivers/built-in.o: In function `of_add_memory':
/home/schurig/d/mkarm/barebox/drivers/of/base.c:1716: undefined
reference to `of_add_memory_bank'
collect2: error: ld returned 1 exit status
make: *** [barebox] Error 1

To be honest, I simply assumed "sandbox has no memory banks, so adding
them is futile" and commented them away. Two weeks ago (or so, not
exactly sure) there was talk on the same function here in the mailing
list. At that time there have been some other suggestions, but they
made the MIPS arch not build. Sascha then said he had no further idea.

> CONFIG_OFTREE_MEM_GENERIC to be enabled on sandbox. Would
> this work for you?

Yes, that worked. This now checks even more code than my #ifdeffery
would have checked, so I like it.

Would then this be ok?

--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -4,7 +4,7 @@ config OFTREE

 config OFTREE_MEM_GENERIC
        depends on OFTREE
-       depends on PPC || ARM || ARCH_EFI
+       depends on PPC || ARM || ARCH_EFI || SANDBOX
        def_bool y



Unrelated: which static checkers use you in non-kernel project?  I
know sparse, but that isn't really usable outisde the kernel, or is
it?

I found clang's static checker giving out lots of warning, similar to
-Wall -Weverything, e.g. he spits out things that aren't technically
errors, but not clean code (e.g. assignment to a variable that is not
used afterwards). But in my own code he found via reasoning some paths
where a null-pointer dereference could happen, or a division by zero.
That's something I don't want. BTW, clangs static checker finds also
both things in barebox, I've had not yet the time to check if they are
indeed possible. He also finds various places of usage of initialized
variables. Some of them I checked, and they have been true. In the
HTML output he writes exactly what circumstances must be valid to
reach that specific point in the code. You cannot easily see that from
what it spits to stdout.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] sandbox: work around missing of_add_memory_bank()

[Lucas Stach]

Am Dienstag, den 22.07.2014, 12:25 +0200 schrieb Holger Schurig:
> > but this doesn't explain a build failure
> 
> The build failure was
> 
>   LD      barebox
> drivers/built-in.o: In function `of_add_memory':
> /home/schurig/d/mkarm/barebox/drivers/of/base.c:1716: undefined
> reference to `of_add_memory_bank'
> collect2: error: ld returned 1 exit status
> make: *** [barebox] Error 1
> 
> To be honest, I simply assumed "sandbox has no memory banks, so adding
> them is futile" and commented them away. Two weeks ago (or so, not
> exactly sure) there was talk on the same function here in the mailing
> list. At that time there have been some other suggestions, but they
> made the MIPS arch not build. Sascha then said he had no further idea.
> 
> > CONFIG_OFTREE_MEM_GENERIC to be enabled on sandbox. Would
> > this work for you?
> 
> Yes, that worked. This now checks even more code than my #ifdeffery
> would have checked, so I like it.
> 
> Would then this be ok?
> 
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -4,7 +4,7 @@ config OFTREE
> 
>  config OFTREE_MEM_GENERIC
>         depends on OFTREE
> -       depends on PPC || ARM || ARCH_EFI
> +       depends on PPC || ARM || ARCH_EFI || SANDBOX
>         def_bool y
> 
I'm fine with this solution.

> 
> 
> Unrelated: which static checkers use you in non-kernel project?  I
> know sparse, but that isn't really usable outisde the kernel, or is
> it?
> 
Personally I've used smatch (it also needs to run a build), but don't
know how useful this is outside of a kernel/barebox source tree. My
current favorite is cppcheck as it doesn't need a build run and provides
a really good signal-to-noise level, but this also means it might
overlook some potential problems.
 
> I found clang's static checker giving out lots of warning, similar to
> -Wall -Weverything, e.g. he spits out things that aren't technically
> errors, but not clean code (e.g. assignment to a variable that is not
> used afterwards). But in my own code he found via reasoning some paths
> where a null-pointer dereference could happen, or a division by zero.
> That's something I don't want. BTW, clangs static checker finds also
> both things in barebox, I've had not yet the time to check if they are
> indeed possible. He also finds various places of usage of initialized
> variables. Some of them I checked, and they have been true. In the
> HTML output he writes exactly what circumstances must be valid to
> reach that specific point in the code. You cannot easily see that from
> what it spits to stdout.

Good to know. I think I'll give it another go after your patches are
accepted.

Regards,
Lucas

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox