NewFunctionStackAnalysisCmd is not right

[astrelsky]

Describe the bug
Within roughly 2 seconds of looking at NewFunctionStackAnalysisCmd.java it is clear that it is either still unfinished or was never reviewed.

To Reproduce
Steps to reproduce the behavior:

1. Wonder why stack analysis sometimes takes so long.
2. Find the stack analyzer code and follow to the background command it runs.
3. Open NewFunctionStackAnalysisCmd.java and navigate to the analyzeFunction method
4. Confusion ensues.

Expected behavior
I don't think I can say what I expect without being overly blunt and just outright rude.

Additional Context
There is also a nice if (instr.getMnemonicString().equals("LEA")) in createStackPointerVariables which makes perfect sense since this is applied to every processor and not just x86.

It would make much more sense to be using the processor specific ContextEvaluator for the SymbolicPropogator. Oh wait, that's right, you can't because they're all implemented as anonymous inner classes in their constant reference analyzers.

[ryanmkurtz]

Moving this to a discussion until more discrete action items can be formed.

[astrelsky]

What do you mean? It's pretty clear that it's not right.

https://github.com/NationalSecurityAgency/ghidra/blob/master/Ghidra%2FFeatures%2FBase%2Fsrc%2Fmain%2Fjava%2Fghidra%2Fapp%2Fcmd%2Ffunction%2FNewFunctionStackAnalysisCmd.java#L156-L188

It's creating and interating over a stack but only ever adds the original function to it. It then iterates over the array list below when the only possible function that can be added to the list is the original function.

Furthermore it is using x86 specific instruction mnemonics but this command is applied to every processor and is enabled by default except 16bit x86.

[dragonmacher]

The problem with pointing out that it sucks is that is not something that is easy to evaluate and work on for us.

Without putting much effort into understanding the code, it seems like it needs to be rewritten. It is not clear what it actually does in general vs for x86 specifically. We certainly could work on more specific change requests if we understood how it works and what it should do. I think it would be helpful for @emteere to offer his thoughts on this class.

[astrelsky]

Ok. Still I figured it better to at least point out that it probably isn't working as intended or isn't being used as intended.

[dragonmacher]

I've made a career out of fixing and refactoring code. I appreciate you pointing it out.