remove --backend-aggressive-opts and all the related code #7
Conversation
|
Unable to locate .performanceTestingBot config file |
|
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information |
Their most recently public accepted PR is: #5 |
|
Processing PR updates... |
labels-and-badges
bot
added
NO JIRA
|
Thanks @2lambda123 for opening this PR! For COLLABORATOR only :
|
![gitginie[bot]](https://avatars.githubusercontent.com/in/712912?s=60&v=4){kind=small}
There was a problem hiding this comment.
@2lambda123
Thank you for your contribution to this repository! We appreciate your effort in opening pull request.
Happy coding!
Reviewer's Guide by SourceryThis pull request removes the --backend-aggressive-opts option and all related code. This includes the removal of aggressive optimizer classes, configuration parameters, and command-line option handling. The changes span multiple files, including the optimizer manager, configuration handling, and build scripts. File-Level Changes
Tips
|
|
PR Details of @2lambda123 in avast-retdec :
|
|
Important Review skippedAuto reviews are limited to specific labels. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| serdes::serializeBool(writer, JSON_backendEmitCfg, isBackendEmitCfg()); | ||
| serdes::serializeBool(writer, JSON_backendEmitCg, isBackendEmitCg()); | ||
| serdes::serializeBool(writer, JSON_detectStaticCode, isDetectStaticCode()); | ||
| serdes::serializeBool(writer, JSON_backendAggressiveOpts, isBackendAggressiveOpts()); | ||
| serdes::serializeBool(writer, JSON_backendKeepAllBrackets, isBackendKeepAllBrackets()); | ||
| serdes::serializeBool(writer, JSON_backendKeepLibraryFuncs, isBackendKeepLibraryFuncs()); | ||
| serdes::serializeBool(writer, JSON_backendNoTimeVaryingInfo, isBackendNoTimeVaryingInfo()); |
There was a problem hiding this comment.
The serdes::serializeBool function calls are repeated multiple times with similar parameters. This repetition can lead to code bloat and makes the code harder to maintain. Consider refactoring these calls into a loop or a helper function to improve maintainability and reduce redundancy.
| setIsBackendNoOpts( serdes::deserializeBool(val, JSON_backendNoOpts, false) ); | ||
| setIsBackendEmitCfg( serdes::deserializeBool(val, JSON_backendEmitCfg, false) ); | ||
| setIsBackendEmitCg( serdes::deserializeBool(val, JSON_backendEmitCg, false) ); | ||
| setIsBackendAggressiveOpts( serdes::deserializeBool(val, JSON_backendAggressiveOpts, false) ); | ||
| setIsBackendKeepAllBrackets( serdes::deserializeBool(val, JSON_backendKeepAllBrackets, false) ); | ||
| setIsBackendKeepLibraryFuncs( serdes::deserializeBool(val, JSON_backendKeepLibraryFuncs, false) ); | ||
| setIsBackendNoTimeVaryingInfo( serdes::deserializeBool(val, JSON_backendNoTimeVaryingInfo, false) ); |
There was a problem hiding this comment.
The serdes::deserializeBool function calls are repeated multiple times with similar parameters. This repetition can lead to code bloat and makes the code harder to maintain. Consider refactoring these calls into a loop or a helper function to improve maintainability and reduce redundancy.
| Log::phase("alias analysis [" + aliasAnalysis->getId() + "]"); | ||
| initAliasAnalysis(); | ||
|
|
||
| Log::phase("optimizations [" + getTypeOfRunOptimizations() + "]"); | ||
| Log::phase("optimizations"); | ||
| runOptimizations(); | ||
| } |
There was a problem hiding this comment.
The initAliasAnalysis and runOptimizations functions are called without any error handling. If these functions can fail or throw exceptions, it would be prudent to handle such cases to ensure the program can either recover gracefully or provide meaningful error messages.
Recommended Solution: Implement error handling for initAliasAnalysis and runOptimizations to manage potential failures or exceptions.
| OptimizerManager::OptimizerManager(const StringSet &enabledOpts, | ||
| const StringSet &disabledOpts, ShPtr<HLLWriter> hllWriter, | ||
| ShPtr<ValueAnalysis> va, ShPtr<CallInfoObtainer> cio, | ||
| ShPtr<ArithmExprEvaluator> arithmExprEvaluator, | ||
| bool enableAggressiveOpts, bool enableDebug): | ||
| ShPtr<ArithmExprEvaluator> arithmExprEvaluator, bool enableDebug): | ||
| enabledOpts(trimOptimizerSuffix(enabledOpts)), | ||
| disabledOpts(trimOptimizerSuffix(disabledOpts)), | ||
| hllWriter(hllWriter), va(va), cio(cio), | ||
| arithmExprEvaluator(arithmExprEvaluator), | ||
| enableAggressiveOpts(enableAggressiveOpts), enableDebug(enableDebug), | ||
| enableDebug(enableDebug), | ||
| recoverFromOutOfMemory(true), backendRunOpts() { |
There was a problem hiding this comment.
The constructor of OptimizerManager initializes several shared pointers (hllWriter, va, cio, arithmExprEvaluator) without checking if they are valid (non-null). Although PRECONDITION_NON_NULL is used, it is better to handle these cases explicitly to avoid potential null pointer dereferences.
Recommendation: Add explicit checks for the validity of these shared pointers and handle the error cases appropriately, possibly by throwing an exception or using an assertion.
| return enabledOpts.empty(); | ||
| } |
There was a problem hiding this comment.
The function isEnabled() returns true if enabledOpts is empty. This could lead to unintended behavior if the caller expects specific optimizations to be enabled but none are provided.
Recommendation: Consider adding a warning or error message when enabledOpts is empty to inform the user that no optimizations will be run. This can help in debugging and ensuring that the function behaves as expected.
| { | ||
| params.setIsBackendEmitCg(true); | ||
| } | ||
| else if (isParam(i, "", "--backend-aggressive-opts")) | ||
| { | ||
| params.setIsBackendAggressiveOpts(true); | ||
| } | ||
| else if (isParam(i, "", "--backend-keep-all-brackets")) | ||
| { | ||
| params.setIsBackendKeepAllBrackets(true); |
There was a problem hiding this comment.
The isParam function is used to check for command-line parameters, but there is no error handling for unrecognized parameters. This could lead to silent failures or unexpected behavior.
Recommendation: Implement a default case or error handling mechanism to catch unrecognized parameters and provide feedback to the user.
| [--backend-no-opts] Disables backend optimizations. | ||
| [--backend-emit-cfg] Emits a CFG for each function in the backend IR (in the .dot format). | ||
| [--backend-emit-cg] Emits a CG for the decompiled module in the backend IR (in the .dot format). | ||
| [--backend-aggressive-opts] Enables aggressive optimizations. | ||
| [--backend-keep-all-brackets] Keeps all brackets in the generated code. | ||
| [--backend-keep-library-funcs] Keep functions from standard libraries. | ||
| [--backend-no-time-varying-info] Do not emit time-varying information, like dates. |
There was a problem hiding this comment.
The command-line options are documented, but there is no validation to ensure that mutually exclusive options are not used together. For example, --backend-no-opts and --backend-emit-cfg might conflict.
Recommendation: Add validation logic to check for mutually exclusive options and provide appropriate error messages to the user.
There was a problem hiding this comment.
@2lambda123
Thank you for your contribution to this repository! We appreciate your effort in closing pull request.
Happy coding!
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @2lambda123 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Could you provide more context on why these aggressive optimizations are being removed? Understanding the rationale behind this change would be helpful.
- Have you conducted any performance benchmarks to assess the impact of removing these optimizations? If so, could you share the results?
- The PR description is quite minimal for such a significant change. Please update it with more details about the motivation, potential impacts, and any relevant background information.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
PR summaryThis Pull Request removes the SuggestionConsider updating the documentation to reflect the removal of the Disclaimer: This comment was entirely generated using AI. Be aware that the information provided may be incorrect. Current plan usage: 8.74% Have feedback or need help? |
Description
Related Issue
Types of changes
Checklist:
Summary by Sourcery
Remove the --backend-aggressive-opts flag and all related code, including the aggressive optimizers and their associated configurations and references.