[lldb][X86] Fix setting target features in ClangExpressionParser

[kovdan01]

Currently, for x86 and x86_64 triples, "+sse" and "+sse2" are appended to Features vector of TargetOptions unconditionally. This vector is later reset in TargetInfo::CreateTargetInfo and filled using info from FeaturesAsWritten vector, so previous modifications of the Features vector have no effect. For x86_64 triple, we append "sse2" unconditionally in X86TargetInfo::initFeatureMap, so despite the Features vector reset, we still have the desired sse features enabled. The corresponding code in X86TargetInfo::initFeatureMap is marked as FIXME, so we should not probably rely on it and should set desired features properly in ClangExpressionParser.

This patch changes the vector the features are appended to from Features to FeaturesAsWritten. It's not reset later and is used to compute resulting Features vector.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Daniil Kovalev (kovdan01)

Changes

Currently, for x86 and x86_64 triples, "+sse" and "+sse2" are appended to Features vector of TargetOptions unconditionally. This vector is later reset in TargetInfo::CreateTargetInfo and filled using info from FeaturesAsWritten vector, so previous modifications of the Features vector have no effect. For x86_64 triple, we append "sse2" unconditionally in X86TargetInfo::initFeatureMap, so despite the Features vector reset, we still have the desired sse features enabled. The corresponding code in X86TargetInfo::initFeatureMap is marked as FIXME, so we should not probably rely on it and should set desired features properly in ClangExpressionParser.

This patch changes the vector the features are appended to from Features to FeaturesAsWritten. It's not reset later and is used to compute resulting Features vector.

---

Full diff: https://github.com/llvm/llvm-project/pull/82364.diff

1 Files Affected:

• (modified) lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp (+2-2)

diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
index 574d661e2a215e..822d286cd6c3c4 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -445,8 +445,8 @@ ClangExpressionParser::ClangExpressionParser(
   // Supported subsets of x86
   if (target_machine == llvm::Triple::x86 ||
       target_machine == llvm::Triple::x86_64) {
-    m_compiler->getTargetOpts().Features.push_back("+sse");
-    m_compiler->getTargetOpts().Features.push_back("+sse2");
+    m_compiler->getTargetOpts().FeaturesAsWritten.push_back("+sse");
+    m_compiler->getTargetOpts().FeaturesAsWritten.push_back("+sse2");
   }
 
   // Set the target CPU to generate code for. This will be empty for any CPU

[adrian-prantl]

Would this change be observable by a test?

[kovdan01]

Would this change be observable by a test?

@adrian-prantl Theoretically, it should be: in ClangExpressionParser::ClangExpressionParser, we try to hardcode sse and sse2 for both x86 and x86_64, while in X86TargetInfo::initFeatureMap, sse2 (implying sse) is only hardcoded for x86_64. So, for x86 the observable behavior should change.

Unfortunately, I'm not sure where and how this could be tested. I suppose the proper place for such a test is somewhere in lldb/unittests/Expression, but I don't see existing tests which check similar stuff. Please let me know if I miss something.

@Michael137 would be glad to see your thoughts on this.

[adrian-prantl]

I'll defer to @Michael137 but if he's okay with this, I'm fine with it.

[Michael137]

Using FeaturesAsWritten makes sense here, thanks! We really should change this logic to derive all the compiler options using the driver, but that's a bigger change for the future