Fix handling of macro definitions on command line

Setting a variable FooBar=2 on the command line explicitly defines the
macro FooBar, but also implicitly defines the macro FOOBAR.

Explicitly set macros are not overridden. The first definition wins.
Implicitly set macros are overridden. The last definition wins.

Fixes: QTCREATORBUG-22176
Change-Id: I5324ba20295a1fade5e98302d698a826400fa80c
Reviewed-by: Oliver Wolff <oliver.wolff@qt.io>

7 files changed, 177 insertions, 36 deletions

diff --git a/src/jomlib/macrotable.cpp b/src/jomlib/macrotable.cpp
index 90a35c5..7026625 100644
--- a/src/jomlib/macrotable.cpp
+++ b/src/jomlib/macrotable.cpp
@@ -56,8 +56,14 @@ QString MacroTable::macroValue(const QString& macroName) const
 void MacroTable::defineEnvironmentMacroValue(const QString& name, const QString& value, bool readOnly)
 {
     const QString upperName = name.toUpper();
-    if (m_macros.contains(upperName))
+    if (m_macros.contains(upperName)) {
+        MacroData &md = m_macros[upperName];
+        if (md.source == MacroSource::CommandLine) {
+            md.source = MacroSource::Environment;
+            setEnvironmentVariable(upperName, expandMacros(md.value));
+        }
         return;
+    }
     QString expandedValue;
     try {
         // The make variable gets the unexpanded value.
@@ -72,11 +78,31 @@ void MacroTable::defineEnvironmentMacroValue(const QString& name, const QString&
     MacroData* macroData = internalSetMacroValue(upperName, value);
     if (!macroData)
         return;
-    macroData->isEnvironmentVariable = true;
+    macroData->source = MacroSource::Environment;
     macroData->isReadOnly = readOnly;
     setEnvironmentVariable(upperName, expandedValue);
 }
 
+void MacroTable::defineCommandLineMacroValue(const QString &name, const QString &value)
+{
+    defineCommandLineMacroValueImpl(name, value, MacroSource::CommandLine);
+}
+
+void MacroTable::defineImplicitCommandLineMacroValue(const QString &name, const QString &value)
+{
+    defineCommandLineMacroValueImpl(name, value, MacroSource::CommandLineImplicit);
+}
+
+void MacroTable::defineCommandLineMacroValueImpl(const QString &name, const QString &value,
+                                                 MacroSource source)
+{
+    MacroData* macroData = internalSetMacroValue(name, value, true);
+    if (!macroData)
+        return;
+    macroData->source = source;
+    macroData->isReadOnly = true;
+}
+
 bool MacroTable::isMacroNameValid(const QString& name) const
 {
     static QRegExp rexMacroIdentifier;
@@ -97,16 +123,27 @@ bool MacroTable::isMacroNameValid(const QString& name) const
  */
 void MacroTable::setMacroValue(const QString& name, const QString& value)
 {
+    setMacroValueImpl(name, value, MacroSource::MakeFile);
+}
+
+void MacroTable::setMacroValueImpl(const QString &name, const QString &value, MacroSource source)
+{
     MacroData* macroData = internalSetMacroValue(name, value);
     if (!macroData) {
         QString msg = QLatin1String("macro name %1 is invalid");
         throw Exception(msg.arg(name));
     }
 
-    if (macroData->isEnvironmentVariable)
+    macroData->source = source;
+    if (macroData->source == MacroSource::Environment)
         setEnvironmentVariable(name, expandMacros(macroData->value));
 }
 
+void MacroTable::predefineValue(const QString &name, const QString &value)
+{
+    setMacroValueImpl(name, value, MacroSource::Predefinition);
+}
+
 /**
  * Sets the value of an environment variable.
  * The environment will be passed to the QProcess instances.
@@ -150,7 +187,8 @@ private:
     const QString &str;
 };
 
-MacroTable::MacroData* MacroTable::internalSetMacroValue(const QString& name, const QString& value)
+MacroTable::MacroData* MacroTable::internalSetMacroValue(const QString &name, const QString &value,
+                                                         bool ignoreReadOnly)
 {
     QString expandedName = expandMacros(name);
     if (!isMacroNameValid(expandedName))
@@ -162,7 +200,7 @@ MacroTable::MacroData* MacroTable::internalSetMacroValue(const QString& name, co
     replaceStringWithLazyValue(newValue, instantiatedName, MacroValueOp(this, expandedName));
 
     result = &m_macros[expandedName];
-    if (!result->isReadOnly)
+    if (ignoreReadOnly || !result->isReadOnly)
         result->value = newValue;
 
     return result;
diff --git a/src/jomlib/macrotable.h b/src/jomlib/macrotable.h
index 27728fd..7a93043 100644
--- a/src/jomlib/macrotable.h
+++ b/src/jomlib/macrotable.h
@@ -48,9 +48,14 @@ public:
     bool isMacroNameValid(const QString& name) const;
     QString macroValue(const QString& macroName) const;
     void defineEnvironmentMacroValue(const QString& name, const QString& value, bool readOnly = false);
+    void defineCommandLineMacroValue(const QString &name, const QString &value);
+    void defineImplicitCommandLineMacroValue(const QString &name, const QString &value);
     void setMacroValue(const QString& name, const QString& value);
     void setMacroValue(const char *szStr, const QString& value) { setMacroValue(QString::fromLatin1(szStr), value); }
     void setMacroValue(const char *szStr, const char *szValue) { setMacroValue(QString::fromLatin1(szStr), QString::fromLatin1(szValue)); }
+    void predefineValue(const QString &name, const QString &value);
+    void predefineValue(const char *szStr, const QString &value) { predefineValue(QString::fromLatin1(szStr), value); }
+    void predefineValue(const char *szStr, const char *szValue) { predefineValue(QString::fromLatin1(szStr), QString::fromLatin1(szValue)); }
     void undefineMacro(const QString& name);
     QString expandMacros(const QString& str, bool inDependentsLine = false) const;
     void dump() const;
@@ -65,18 +70,27 @@ public:
     static void applySubstitution(const Substitution &substitution, QString &value);
 
 private:
-    struct MacroData
+    enum class MacroSource
     {
-        MacroData()
-            : isEnvironmentVariable(false), isReadOnly(false)
-        {}
+        CommandLine,
+        CommandLineImplicit,
+        MakeFile,
+        Environment,
+        Predefinition
+    };
 
-        bool isEnvironmentVariable;
-        bool isReadOnly;
+    struct MacroData
+    {
+        MacroSource source = MacroSource::MakeFile;
+        bool isReadOnly = false;
         QString value;
     };
 
-    MacroData* internalSetMacroValue(const QString& name, const QString& value);
+    void setMacroValueImpl(const QString &name, const QString &value, MacroSource source);
+    void defineCommandLineMacroValueImpl(const QString &name, const QString &value,
+                                         MacroSource source);
+    MacroData* internalSetMacroValue(const QString &name, const QString &value,
+                                     bool ignoreReadOnly = false);
     void setEnvironmentVariable(const QString& name, const QString& value);
     QString expandMacros(const QString& str, bool inDependentsLine, QSet<QString>& usedMacros) const;
     QString cycleCheckedMacroValue(const QString& macroName, QSet<QString>& usedMacros) const;
diff --git a/src/jomlib/makefilefactory.cpp b/src/jomlib/makefilefactory.cpp
index c97243b..1fb0a8a 100644
--- a/src/jomlib/makefilefactory.cpp
+++ b/src/jomlib/makefilefactory.cpp
@@ -133,26 +133,26 @@ bool MakefileFactory::apply(const QStringList& commandLineArguments, Options **o
 
     readEnvironment(m_environment, macroTable, options->overrideEnvVarMacros);
     if (!options->ignorePredefinedRulesAndMacros) {
-        macroTable->setMacroValue("MAKE", encloseInDoubleQuotesIfNeeded(options->fullAppPath));
-        macroTable->setMacroValue("MAKEDIR", encloseInDoubleQuotesIfNeeded(QDir::currentPath()));
-        macroTable->setMacroValue("AS", "ml");       // Macro Assembler
-        macroTable->setMacroValue("ASFLAGS", QString());
-        macroTable->setMacroValue("BC", "bc");       // Basic Compiler
-        macroTable->setMacroValue("BCFLAGS", QString());
-        macroTable->setMacroValue("CC", "cl");       // C Compiler
-        macroTable->setMacroValue("CCFLAGS", QString());
-        macroTable->setMacroValue("COBOL", "cobol"); // COBOL Compiler
-        macroTable->setMacroValue("COBOLFLAGS", QString());
-        macroTable->setMacroValue("CPP", "cl");      // C++ Compiler
-        macroTable->setMacroValue("CPPFLAGS", QString());
-        macroTable->setMacroValue("CXX", "cl");      // C++ Compiler
-        macroTable->setMacroValue("CXXFLAGS", QString());
-        macroTable->setMacroValue("FOR", "fl");      // FORTRAN Compiler
-        macroTable->setMacroValue("FORFLAGS", QString());
-        macroTable->setMacroValue("PASCAL", "pl");   // Pascal Compiler
-        macroTable->setMacroValue("PASCALFLAGS", QString());
-        macroTable->setMacroValue("RC", "rc");       // Resource Compiler
-        macroTable->setMacroValue("RCFLAGS", QString());
+        macroTable->predefineValue("MAKE", encloseInDoubleQuotesIfNeeded(options->fullAppPath));
+        macroTable->predefineValue("MAKEDIR", encloseInDoubleQuotesIfNeeded(QDir::currentPath()));
+        macroTable->predefineValue("AS", "ml");       // Macro Assembler
+        macroTable->predefineValue("ASFLAGS", QString());
+        macroTable->predefineValue("BC", "bc");       // Basic Compiler
+        macroTable->predefineValue("BCFLAGS", QString());
+        macroTable->predefineValue("CC", "cl");       // C Compiler
+        macroTable->predefineValue("CCFLAGS", QString());
+        macroTable->predefineValue("COBOL", "cobol"); // COBOL Compiler
+        macroTable->predefineValue("COBOLFLAGS", QString());
+        macroTable->predefineValue("CPP", "cl");      // C++ Compiler
+        macroTable->predefineValue("CPPFLAGS", QString());
+        macroTable->predefineValue("CXX", "cl");      // C++ Compiler
+        macroTable->predefineValue("CXXFLAGS", QString());
+        macroTable->predefineValue("FOR", "fl");      // FORTRAN Compiler
+        macroTable->predefineValue("FORFLAGS", QString());
+        macroTable->predefineValue("PASCAL", "pl");   // Pascal Compiler
+        macroTable->predefineValue("PASCALFLAGS", QString());
+        macroTable->predefineValue("RC", "rc");       // Resource Compiler
+        macroTable->predefineValue("RCFLAGS", QString());
     }
 
     try {
diff --git a/src/jomlib/options.cpp b/src/jomlib/options.cpp
index b026cf6..01ce796 100644
--- a/src/jomlib/options.cpp
+++ b/src/jomlib/options.cpp
@@ -30,8 +30,9 @@
 
 #include <cstdlib>
 
-#include <QThread>
 #include <QFile>
+#include <QSet>
+#include <QThread>
 
 namespace NMakeFile {
 
@@ -85,6 +86,7 @@ bool Options::readCommandLineArguments(QStringList arguments, QString& makefile,
     QString makeflags;
     if (!expandCommandFiles(arguments))
         return false;
+    QSet<QString> explicitlyDefinedMacros;
 
     const QStringList originalArguments = arguments;
     while (!arguments.isEmpty()) {
@@ -105,7 +107,16 @@ bool Options::readCommandLineArguments(QStringList arguments, QString& makefile,
                 fprintf(stderr, "Error: The macro name %s is invalid.", qPrintable(name));
                 exit(128);
             }
-            macroTable.defineEnvironmentMacroValue(name, trimLeft(arg.mid(idx+1)), true);
+            const QString value = trimLeft(arg.mid(idx+1));
+            if (!explicitlyDefinedMacros.contains(name)) {
+                explicitlyDefinedMacros.insert(name);
+                macroTable.defineCommandLineMacroValue(name, value);
+            }
+            const QString upperName = name.toUpper();
+            if (upperName != name && !explicitlyDefinedMacros.contains(upperName)) {
+                explicitlyDefinedMacros.remove(upperName);
+                macroTable.defineImplicitCommandLineMacroValue(upperName, value);
+            }
         } else {
             // handle target
             arg = arg.trimmed();
diff --git a/tests/makefiles/blackbox/macrosOnCommandLine/test.mk b/tests/makefiles/blackbox/macrosOnCommandLine/test.mk
new file mode 100644
index 0000000..efd7090
--- /dev/null
+++ b/tests/makefiles/blackbox/macrosOnCommandLine/test.mk
@@ -0,0 +1,3 @@
+FooBar = 1
+!message FooBar:$(FooBar)
+!message FOOBAR:$(FOOBAR)
diff --git a/tests/tests.cpp b/tests/tests.cpp
index d939cb0..21184de 100644
--- a/tests/tests.cpp
+++ b/tests/tests.cpp
@@ -25,11 +25,12 @@
 
 #include "tests.h"
 
-#include <QTest>
+#include <QDebug>
 #include <QDir>
+#include <QHash>
 #include <QScopedPointer>
-#include <QDebug>
 #include <QStringBuilder>
+#include <QTest>
 
 #include <ppexprparser.h>
 #include <makefilefactory.h>
@@ -39,6 +40,7 @@
 #include <exception.h>
 
 #include <algorithm>
+#include <functional>
 #include <limits>
 
 using namespace NMakeFile;
@@ -1208,6 +1210,77 @@ void Tests::suffixes()
     QCOMPARE(output.takeFirst(), QByteArray("c -> x"));
 }
 
+using ByteArrayDict = QHash<QByteArray, QByteArray>;
+
+void Tests::macrosOnCommandLine_data()
+{
+    QTest::addColumn<QStringList>("arguments");
+    QTest::addColumn<ByteArrayDict>("expectedMacros");
+    QTest::newRow("no_arguments")
+            << QStringList()
+            << ByteArrayDict{ { "FooBar", "1" }, { "FOOBAR", "" } };
+    QTest::newRow("FooBar")
+            << QStringList{ "FooBar=2" }
+            << ByteArrayDict{ { "FooBar", "2" }, { "FOOBAR", "2" } };
+    QTest::newRow("FOOBAR")
+            << QStringList{ "FOOBAR=2" }
+            << ByteArrayDict{ { "FooBar", "1" }, { "FOOBAR", "2" } };
+    QTest::newRow("foobar")
+            << QStringList{ "foobar=2" }
+            << ByteArrayDict{ { "FooBar", "1" }, { "FOOBAR", "2" } };
+    QTest::newRow("FooBar_FOOBAR")
+            << QStringList{ "FooBar=2", "FOOBAR=3" }
+            << ByteArrayDict{ { "FooBar", "2" }, { "FOOBAR", "3" } };
+    QTest::newRow("FOOBAR_FooBar")
+            << QStringList{ "FOOBAR=2", "FooBar=3" }
+            << ByteArrayDict{ { "FooBar", "3" }, { "FOOBAR", "2" } };
+    QTest::newRow("FooBar_FOOBAR_foobar")
+            << QStringList{ "FooBar=2", "FOOBAR=3", "foobar=4" }
+            << ByteArrayDict{ { "FooBar", "2" }, { "FOOBAR", "3" } };
+    QTest::newRow("foobar_FooBar_FOOBAR")
+            << QStringList{ "foobar=2", "FooBar=3", "FOOBAR=4" }
+            << ByteArrayDict{ { "FooBar", "3" }, { "FOOBAR", "4" } };
+    QTest::newRow("FooBar_FooBar")
+            << QStringList{ "FooBar=2", "FooBar=3" }
+            << ByteArrayDict{ { "FooBar", "2" }, { "FOOBAR", "3" } };
+    QTest::newRow("FooBar_FooBar_FooBar")
+            << QStringList{ "FooBar=2", "FooBar=3", "FooBar=4" }
+            << ByteArrayDict{ { "FooBar", "2" }, { "FOOBAR", "4" } };
+    QTest::newRow("FOOBAR_FOOBAR")
+            << QStringList{ "FOOBAR=2", "FOOBAR=3" }
+            << ByteArrayDict{ { "FooBar", "1" }, { "FOOBAR", "2" } };
+    QTest::newRow("FOOBAR_FOOBAR_FOOBAR")
+            << QStringList{ "FOOBAR=2", "FOOBAR=3", "FOOBAR=4" }
+            << ByteArrayDict{ { "FooBar", "1" }, { "FOOBAR", "2" } };
+    QTest::newRow("FooBar_FooBar_FOOBAR_FOOBAR")
+            << QStringList{ "FooBar=2", "FooBar=3", "FOOBAR=4", "FOOBAR=5" }
+            << ByteArrayDict{ { "FooBar", "2" }, { "FOOBAR", "4" } };
+}
+
+void Tests::macrosOnCommandLine()
+{
+    QFETCH(QStringList, arguments);
+    QFETCH(ByteArrayDict, expectedMacros);
+    QVERIFY(runJom(arguments << "/nologo" << "/f" << "test.mk", "blackbox/macrosOnCommandLine"));
+    QCOMPARE(m_jomProcess->exitCode(), 0);
+    QList<QByteArray> output = m_jomProcess->readAllStandardOutput().split('\n');
+    std::transform(output.cbegin(), output.cend(), output.begin(),
+                   [](const QByteArray &line) { return line.trimmed(); });
+    auto it = std::remove_if(output.begin(), output.end(), std::mem_fn(&QByteArray::isEmpty));
+    if (it != output.end())
+        output.erase(it, output.end());
+    ByteArrayDict macros;
+    for (const QByteArray &line : qAsConst(output)) {
+        auto x = line.split(':');
+        macros[x.at(0)] = x.at(1);
+    }
+    if (macros != expectedMacros) {
+        qDebug() << "  actual:" << macros;
+        qDebug() << "expected:" << expectedMacros;
+        QFAIL("Unexpected macro values");
+    }
+}
+
 void Tests::nonexistentDependent()
 {
     QVERIFY(runJom(QStringList() << "/nologo" << "/f" << "test.mk", "blackbox/nonexistentdependent"));
diff --git a/tests/tests.h b/tests/tests.h
index b98758c..820a078 100644
--- a/tests/tests.h
+++ b/tests/tests.h
@@ -80,6 +80,8 @@ private slots:
     void builtin_cd_data();
     void builtin_cd();
     void suffixes();
+    void macrosOnCommandLine_data();
+    void macrosOnCommandLine();
     void nonexistentDependent();
     void noTargets();
     void outOfDateCheck();