Re: git: 805d759338a2 - main - mlx4: Move DEFINE_MUTEX() outside function body.

On 21 May 2023, at 22:37, Hans Petter Selasky <hselasky@freebsd.org> wrote:
> 
> On 5/21/23 21:41, Jessica Clarke wrote:
>>> One difference between Linux and FreeBSD when doing the monotolithic kernel build: FreeBSD puts all object files resulting from compilation into the same object directory. This implies there cannot be two object files sharing the same name as a result of compilation. This in turn implies all source file names are unique across the whole of "sys/*", because converting a source file name into the corresponding object file name, is simply done by changing the suffix of the file in question.
> 
> Hi Jess,
> 
>> I am aware of all of this. Please do not talk to me like I am an idiot. Your tone here is extremely patronising.
> 
> I can not know what you know before you tell me. I'm not a mind reader and I for sure don't think you are an idiot. Those are in such case your own words about yourself and not mine, to make it clear.

You can’t know, though you could go look at commit history to see that I have made changes to the build system and thus probably have an understanding. Or you could assume that I know what I’m talking about when I choose to engage. Both are good things to do that show some amount of respect, otherwise you are at risk of insulting people by assuming that you need to explain the basics to them.

>>> If you are worried multiple DEFINE_MUTEX(lock) will result in multiple global symbols having the same "lock" name, all you need to do is pass through the ${.TARGET} variable from "make" as a define, stripping a few invalid characters, and macro-concat that to the locally generated global variable name, and you are all good.
> 
>> You could. But that’s a pretty gross hack IMO, and depending on how you do it may still not be unique. Not to mention it’s going to further bloat the symbol tables with these long names.
> 
> Why do you think I don't know what a hack is?

I didn’t. I said “IMO”. *My* opinion.

> In the case about the "lock" name being global:
> 
> - My solution is to pass a define to the compiler to help make the identifier unique across the monotolithic kernel build.
> 
> - When building the LINT target, the linker will fail if there are duplicate global symbols. Then the debug information output by the linker will tell the file and line. Go there and change the identifier. It should be rare. Problem solved.
> 
> - All symbols being made global for the sake of processing them, are listed by name. And after the final sorted sysinit table is created and linked, all those symbol names can be stripped away from the resulting binary, and then only the relocations remain, if I'm not mistaken. I didn't do that in my differential review, because it is a prototype. The argument about symbol table bloat, because of prefixing or suffixing global symbols is not valid the way I see it.
> 
>> Given the sysinit subsystem still needs to be able to merge in new sysinits registered dynamically, one might as well just do the sort dynamically, it’s very little added complexity on top, especially when sorting functions are just a libkern call away. Much less complexity than all this scripting to generate tables.
> 
> Do you agree about the following statement or not?
> 
> If you have two already sorted lists and a pointer to each of them, then:
> 
> Method a) place the two lists after each other in memory and then call qsort() on the resulting list, is relatively slow.
> 
> Method b) look at the next element from each list, and store the smallest one into the destination list, and repeat until the end of both source lists are reached, is relatively much faster.

Clearly merging is faster. But compared with qsort it’s unlikely to be all that significant; lg n grows slowly.

>>> Your comment is correct based on your prior knowledge about Linux and compilers. But at this point FreeBSD is different. That's why porting code from Linux to FreeBSD is sometimes difficult, because you need to keep track of how source files are renamed. You cannot just use "meld Linux/blah FreeBSD/blah" to compare directories ...
>> DEFINE_MUTEX is Linux’s API, implemented in LinuxKPI. So Linux’s behaviour is absolutely relevant. Any deviation from Linux’s semantics is an incompatibility that requires patching any sources that are built for FreeBSD using LinuxKPI. It is generally best to minimise that.
> 
> One side of this is compile time issues. If a developer needs to change something to make the code compile for FreeBSD, that's acceptable. And then the developer can also say the code was made for FreeBSD and not Linux, which is good with regards to the GPLv2 license, and not just copying solutions from Linux or being force to implement things exactly like on Linux, so-called bug-by-bug compliance.
> 
> The other side is runtime issues. Here the goal should be to minimise issues to zero and try to outperform Linux.

I don’t have any clue what you’re going on about here. Tweaking Linux code to be compatible with FreeBSD’s LinuxKPI doesn’t suddenly render the code free from GPLv2, it’s still Linux-derived code, but you’ve had to modify it rather than be able to use it as-is, which is the goal of LinuxKPI, where possible. And so ideally *no* changes would be acceptable. So I disagree with your statement that needing to change things is acceptable; it’s only acceptable where there is no good alternative.

I don’t see what “outperform Linux” has to do with any of this discussion.

>> So, once again, I am asking you to revert your change.
> 
> I still cannot see why this change should be reverted.

Because it serves no purpose today, and should always remain valid code, because Linux’s API makes that valid, and thus LinuxKPI should ensure it’s valid too. If the change is required then LinuxKPI's compatibility has been broken, which is a regression that should not be permitted. Therefore it is totally unnecessary, and if anything is worse because it unnecessarily increases the scope of a variable that is only needed within a single function.

> Does the following code example and compiler warning explain why this change should remain:
> 
> # cat << EOF > test.c
> int
> main()
> {
>        extern char string[];
>        char string[] = { "test" };
> 
>        return (0);
> }
> EOF
> 
> # cc -O2 -Wall test.c
> test.c:11:7: error: non-extern declaration of 'string' follows extern declaration
>        char string[] = { "test" };
>             ^
> test.c:10:14: note: previous declaration is here
>        extern char string[];
> 
> 
> 
> Like my commit message says, you cannot declare global variables on the stack. When variables are on the stack, they are inside a function body. There is no stack outside any C-functions. It's just another way to say it and what is the problem about that?

I understood what you were *actually* trying to achieve with the change from your first reply. I have never disputed that. What I disputed was your commit message, which at best is extremely misleading and at worst makes no sense, and the necessity of the change. But this case should not be hit here, because if it is then you’ve broken LinuxKPI.

Please revert the commit. It should never be necessary. I’m not going to ask a fourth time.

Jess