BaseEmitter::commentf() shouldn't format string, which will be discarded by the emitter (fixes #306)

4 files changed, 61 insertions, 32 deletions

diff --git a/src/asmjit/core.h b/src/asmjit/core.h
index 3b50c65..52540ab 100644
--- a/src/asmjit/core.h
+++ b/src/asmjit/core.h
@@ -1307,7 +1307,7 @@ namespace asmjit {
 //! invoking functions. At the moment it's the easiest way of generating code
 //! in AsmJit as most architecture and OS specifics is properly abstracted and
 //! handled by AsmJit automatically. However, abstractions also mean restrictions,
-//! which means that \ref BaseCompiler has more limitations that \ref BaseAssembler
+//! which means that \ref BaseCompiler has more limitations than \ref BaseAssembler
 //! or \ref BaseBuilder.
 //!
 //! Since \ref BaseCompiler provides register allocation it also establishes the
diff --git a/src/asmjit/core/assembler.cpp b/src/asmjit/core/assembler.cpp
index 4cf706f..c0cbf0f 100644
--- a/src/asmjit/core/assembler.cpp
+++ b/src/asmjit/core/assembler.cpp
@@ -366,20 +366,23 @@ Error BaseAssembler::embedLabelDelta(const Label& label, const Label& base, size
 // ============================================================================
 
 Error BaseAssembler::comment(const char* data, size_t size) {
-  if (ASMJIT_UNLIKELY(!_code))
-    return reportError(DebugUtils::errored(kErrorNotInitialized));
-
-#ifndef ASMJIT_NO_LOGGING
-  if (_logger) {
-    _logger->log(data, size);
-    _logger->log("\n", 1);
+  if (!hasEmitterFlag(kFlagLogComments)) {
+    if (!hasEmitterFlag(kFlagAttached))
+      return reportError(DebugUtils::errored(kErrorNotInitialized));
     return kErrorOk;
   }
+
+#ifndef ASMJIT_NO_LOGGING
+  // Logger cannot be NULL if `kFlagLogComments` is set.
+  ASMJIT_ASSERT(_logger != nullptr);
+
+  _logger->log(data, size);
+  _logger->log("\n", 1);
+  return kErrorOk;
 #else
   DebugUtils::unused(data, size);
-#endif
-
   return kErrorOk;
+#endif
 }
 
 // ============================================================================
diff --git a/src/asmjit/core/emitter.cpp b/src/asmjit/core/emitter.cpp
index 23ac3b3..f684140 100644
--- a/src/asmjit/core/emitter.cpp
+++ b/src/asmjit/core/emitter.cpp
@@ -66,22 +66,35 @@ Error BaseEmitter::finalize() {
 // [asmjit::BaseEmitter - Internals]
 // ============================================================================
 
-static constexpr uint32_t kEmitterPreservedFlags =
-    BaseEmitter::kFlagOwnLogger |
-    BaseEmitter::kFlagOwnErrorHandler ;
+static constexpr uint32_t kEmitterPreservedFlags = BaseEmitter::kFlagOwnLogger | BaseEmitter::kFlagOwnErrorHandler;
 
 static ASMJIT_NOINLINE void BaseEmitter_updateForcedOptions(BaseEmitter* self) noexcept {
-  bool hasLogger = self->_logger != nullptr;
-  bool hasValidationOptions;
+  bool emitComments = false;
+  bool hasValidationOptions = false;
 
-  if (self->emitterType() == BaseEmitter::kTypeAssembler)
+  if (self->emitterType() == BaseEmitter::kTypeAssembler) {
+    // Assembler: Don't emit comments if logger is not attached.
+    emitComments = self->_code != nullptr && self->_logger != nullptr;
     hasValidationOptions = self->hasValidationOption(BaseEmitter::kValidationOptionAssembler);
-  else
+  }
+  else {
+    // Builder/Compiler: Always emit comments, we cannot assume they won't be used.
+    emitComments = self->_code != nullptr;
     hasValidationOptions = self->hasValidationOption(BaseEmitter::kValidationOptionIntermediate);
+  }
 
-  self->_forcedInstOptions &= ~BaseInst::kOptionReserved;
-  if (hasLogger || hasValidationOptions)
+  if (emitComments)
+    self->_addEmitterFlags(BaseEmitter::kFlagLogComments);
+  else
+    self->_clearEmitterFlags(BaseEmitter::kFlagLogComments);
+
+  // The reserved option tells emitter (Assembler/Builder/Compiler) that there
+  // may be either a border case (CodeHolder not attached, for example) or that
+  // logging or validation is required.
+  if (self->_code == nullptr || self->_logger || hasValidationOptions)
     self->_forcedInstOptions |= BaseInst::kOptionReserved;
+  else
+    self->_forcedInstOptions &= ~BaseInst::kOptionReserved;
 }
 
 // ============================================================================
@@ -306,15 +319,22 @@ ASMJIT_FAVOR_SIZE Error BaseEmitter::emitArgsAssignment(const FuncFrame& frame,
 // ============================================================================
 
 Error BaseEmitter::commentf(const char* fmt, ...) {
-  if (ASMJIT_UNLIKELY(!_code))
-    return DebugUtils::errored(kErrorNotInitialized);
+  if (!hasEmitterFlag(kFlagLogComments)) {
+    if (!hasEmitterFlag(kFlagAttached))
+      return reportError(DebugUtils::errored(kErrorNotInitialized));
+    return kErrorOk;
+  }
 
 #ifndef ASMJIT_NO_LOGGING
+  StringTmp<1024> sb;
+
   va_list ap;
   va_start(ap, fmt);
-  Error err = commentv(fmt, ap);
+  Error err = sb.appendVFormat(fmt, ap);
   va_end(ap);
-  return err;
+
+  ASMJIT_PROPAGATE(err);
+  return comment(sb.data(), sb.size());
 #else
   DebugUtils::unused(fmt);
   return kErrorOk;
@@ -322,16 +342,17 @@ Error BaseEmitter::commentf(const char* fmt, ...) {
 }
 
 Error BaseEmitter::commentv(const char* fmt, va_list ap) {
-  if (ASMJIT_UNLIKELY(!_code))
-    return DebugUtils::errored(kErrorNotInitialized);
+  if (!hasEmitterFlag(kFlagLogComments)) {
+    if (!hasEmitterFlag(kFlagAttached))
+      return reportError(DebugUtils::errored(kErrorNotInitialized));
+    return kErrorOk;
+  }
 
 #ifndef ASMJIT_NO_LOGGING
   StringTmp<1024> sb;
   Error err = sb.appendVFormat(fmt, ap);
 
-  if (ASMJIT_UNLIKELY(err))
-    return err;
-
+  ASMJIT_PROPAGATE(err);
   return comment(sb.data(), sb.size());
 #else
   DebugUtils::unused(fmt, ap);
@@ -346,6 +367,7 @@ Error BaseEmitter::commentv(const char* fmt, va_list ap) {
 Error BaseEmitter::onAttach(CodeHolder* code) noexcept {
   _code = code;
   _environment = code->environment();
+  _addEmitterFlags(kFlagAttached);
 
   const ArchTraits& archTraits = ArchTraits::byArch(code->arch());
   uint32_t nativeRegType = Environment::is32Bit(code->arch()) ? BaseReg::kTypeGp32 : BaseReg::kTypeGp64;
@@ -358,16 +380,16 @@ Error BaseEmitter::onAttach(CodeHolder* code) noexcept {
 Error BaseEmitter::onDetach(CodeHolder* code) noexcept {
   DebugUtils::unused(code);
 
-  _clearEmitterFlags(~kEmitterPreservedFlags);
-  _forcedInstOptions = BaseInst::kOptionReserved;
-  _privateData = 0;
-
   if (!hasOwnLogger())
     _logger = nullptr;
 
   if (!hasOwnErrorHandler())
     _errorHandler = nullptr;
 
+  _clearEmitterFlags(~kEmitterPreservedFlags);
+  _forcedInstOptions = BaseInst::kOptionReserved;
+  _privateData = 0;
+
   _environment.reset();
   _gpRegInfo.reset();
 
diff --git a/src/asmjit/core/emitter.h b/src/asmjit/core/emitter.h
index 51651fd..fcb9bb5 100644
--- a/src/asmjit/core/emitter.h
+++ b/src/asmjit/core/emitter.h
@@ -109,6 +109,10 @@ public:
 
   //! Emitter flags.
   enum EmitterFlags : uint32_t {
+    //! Emitter is attached to CodeHolder.
+    kFlagAttached = 0x01u,
+    //! The emitter must emit comments.
+    kFlagLogComments = 0x08u,
     //! The emitter has its own \ref Logger (not propagated from \ref CodeHolder).
     kFlagOwnLogger = 0x10u,
     //! The emitter has its own \ref ErrorHandler (not propagated from \ref CodeHolder).