IR: Properly handle escape characters in Attribute::getAsString()

[honggyu.kim]

Summary

If an attribute name has special characters such as '\01', it is not
properly printed in LLVM assembly language format. Since the format
expects the special characters are printed as it is, it has to contain
escape characters to make it printable.

Before:

attributes #0 = { ... "counting-function"="^A__gnu_mcount_nc" ...

After:

attributes #0 = { ... "counting-function"="\01__gnu_mcount_nc" ...

Signed-off-by: Honggyu Kim <[e-mail]>

[rjmccall]

I'm not sure what you're saying. The existing function PrintEscapedString in AsmWriter.cpp does exactly what you want; you just need to call it from getAsString as well. You'll need to expose it somewhere, of course.

[honggyu.kim]

Hi John,

In D23792#522870, @rjmccall wrote:

I'm not sure what you're saying. The existing function PrintEscapedString in AsmWriter.cpp does exactly what you want; you just need to call it from getAsString as well. You'll need to expose it somewhere, of course.

Let me show you the execution sequence to print attribute list as below.

attributes #0 = { ... "counting-function"="\01__gnu_mcount_nc" ...

llvm/lib/IR/AsmWriter.cpp

void AssemblyWriter::writeAllAttributeGroups() {
  std::vector<std::pair<AttributeSet, unsigned> > asVec;
    ...
  for (const auto &I : asVec)
    Out << "attributes #" << I.second << " = { "
        << I.first.getAsString(AttributeSet::FunctionIndex, true) << " }\n";
    ...
}

llvm/lib/IR/AttributeSetNode.h

typedef const Attribute *iterator;

llvm/lib/IR/Attributes.cpp

std::string AttributeSet::getAsString(unsigned Index, bool InAttrGrp) const {
  AttributeSetNode *ASN = getAttributes(Index);
  return ASN ? ASN->getAsString(InAttrGrp) : std::string("");
}

std::string AttributeSetNode::getAsString(bool InAttrGrp) const {
  std::string Str;
  for (iterator I = begin(), E = end(); I != E; ++I) {
    if (I != begin())
      Str += ' ';
    Str += I->getAsString(InAttrGrp);
  }
  return Str;
}

std::string Attribute::getAsString(bool InAttrGrp) const {
    ...
  // Convert target-dependent attributes to strings of the form:
  //
  //   "kind"
  //   "kind" = "value"
  //
  if (isStringAttribute()) {
    std::string Result;
    Result += (Twine('"') + getKindAsString() + Twine('"')).str();

    StringRef Val = pImpl->getValueAsString();
    if (Val.empty()) return Result;

    Result += ("=\"" + Val + Twine('"')).str();
    return Result;
  }

  llvm_unreachable("Unknown attribute");
}

In Attribute::getAsString(), it is expected to return std::string as the name describes. I think getAsString() function should not print the string directly with PrintEscapedString(). If I directly print attribute string here, we have to modify many functions in the execution paths above. The only problem here is printing Val. That's why I added a new function GetEscapedString(). Please correct me if there's a better approach.

[rjmccall]

In D23792#522872, @honggyu.kim wrote:

In Attribute::getAsString(), it is expected to return std::string as the name describes. I think getAsString() function should not print the string directly with PrintEscapedString(). If I directly print attribute string here, we have to modify many functions in the execution paths above. The only problem here is printing Val. That's why I added a new function GetEscapedString(). Please correct me if there's a better approach.

PrintEscapedString prints to a raw_ostream that's passed in as a parameter, and there's a raw_string_ostream that will just append to a std::string.

John.

[honggyu.kim]

In D23792#524043, @rjmccall wrote:

In D23792#522872, @honggyu.kim wrote:

In Attribute::getAsString(), it is expected to return std::string as the name describes. I think getAsString() function should not print the string directly with PrintEscapedString(). If I directly print attribute string here, we have to modify many functions in the execution paths above. The only problem here is printing Val. That's why I added a new function GetEscapedString(). Please correct me if there's a better approach.

PrintEscapedString prints to a raw_ostream that's passed in as a parameter, and there's a raw_string_ostream that will just append to a std::string.

John.

Thanks for pointing out this. But there is one more tiny but tricky issue here. The implementation of PrintEscapedString and GetEscapedString are a bit different. PrintEscapedString just prints an escaped character '\' itself, but GetEscapedString has to put double quoted string "\\" for later usage. For this purpose, PrintEscapedString has to also be modified as below in that case.

--- a/lib/IR/AsmWriter.cpp
+++ b/lib/IR/AsmWriter.cpp
@@ -345,7 +345,7 @@ static void PrintEscapedString(StringRef Name, raw_ostream &Out) {
     if (isprint(C) && C != '\\' && C != '"')
       Out << C;
     else
-      Out << '\\' << hexdigit(C >> 4) << hexdigit(C & 0x0F);
+      Out << "\\" << hexdigit(C >> 4) << hexdigit(C & 0x0F);
   }
 }

But it will change the behaviour of other functions that use PrintEscapedString, that's why I thought it'd be better to add a new function GetEscapedString to make it simple.

[rjmccall]

In D23792#524795, @honggyu.kim wrote:

In D23792#524043, @rjmccall wrote:

In D23792#522872, @honggyu.kim wrote:

In Attribute::getAsString(), it is expected to return std::string as the name describes. I think getAsString() function should not print the string directly with PrintEscapedString(). If I directly print attribute string here, we have to modify many functions in the execution paths above. The only problem here is printing Val. That's why I added a new function GetEscapedString(). Please correct me if there's a better approach.

PrintEscapedString prints to a raw_ostream that's passed in as a parameter, and there's a raw_string_ostream that will just append to a std::string.

John.

Thanks for pointing out this. But there is one more tiny but tricky issue here. The implementation of PrintEscapedString and GetEscapedString are a bit different. PrintEscapedString just prints an escaped character '\' itself, but GetEscapedString has to put double quoted string "\\" for later usage. For this purpose, PrintEscapedString has to also be modified as below in that case.

--- a/lib/IR/AsmWriter.cpp
+++ b/lib/IR/AsmWriter.cpp
@@ -345,7 +345,7 @@ static void PrintEscapedString(StringRef Name, raw_ostream &Out) {
     if (isprint(C) && C != '\\' && C != '"')
       Out << C;
     else
-      Out << '\\' << hexdigit(C >> 4) << hexdigit(C & 0x0F);
+      Out << "\\" << hexdigit(C >> 4) << hexdigit(C & 0x0F);
   }
 }

But it will change the behaviour of other functions that use PrintEscapedString, that's why I thought it'd be better to add a new function GetEscapedString to make it simple.

These should have the same behavior; only, the existing code should be slightly more efficient (without optimization) because it knows that it's only adding a single character rather than possible an array of them. Are you actually seeing different behavior at runtime?

John.

[honggyu.kim]

In D23792#524931, @rjmccall wrote:

These should have the same behavior; only, the existing code should be slightly more efficient (without optimization) because it knows that it's only adding a single character rather than possible an array of them. Are you actually seeing different behavior at runtime?

John.

Hi John,

You're right. I've updated it by using existing PrintEscapedString() as you explained. I've tested it with mcount.c and gnu-mcount.c, and they're all passed.
I appreciate your guide!

Honggyu

[rjmccall]

Thank you, this looks better. A minor suggestions and then this LGTM.

include/llvm/ADT/StringExtras.h

19 ↗

(On Diff #69230)

You should be able to just forward-declare raw_ostream in this header:

namespace llvm {
class raw_ostream;

lib/IR/Attributes.cpp

394 ↗

(On Diff #69230)

I would suggest just printing this directly into Result, like so:

{
  raw_string_ostream OS(Result);
  OS << "=\"";
  PrintEscapedString(AttrVal, OS);
  OS << "\"";
}

[honggyu.kim]

In D23792#525693, @rjmccall wrote:

Thank you, this looks better. A minor suggestions and then this LGTM.

I've just updated as you suggested.
Thanks for your help.

Honggyu

[rjmccall]

LGTM, thanks!

[honggyu.kim]

Since I don't have write permission, it would be grateful if anyone can give me commit permission on the repository.
Otherwise, can anyone please commit this patch and related ones? - D22825, D22666

Thanks,
Honggyu

llvm/trunk/include/llvm/ADT/StringExtras.h

 #ifndef LLVM_ADT_STRINGEXTRAS_H
 #define LLVM_ADT_STRINGEXTRAS_H
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/DataTypes.h"
 #include <iterator>
 
 namespace llvm {
+class raw_ostream;
 template<typename T> class SmallVectorImpl;
 
 /// hexdigit - Return the hexadecimal character for the
 /// given number \p X (which should be less than 16).
 static inline char hexdigit(unsigned X, bool LowerCase = false) {
   const char HexChar = LowerCase ? 'a' : 'A';
   return X < 10 ? '0' + X : HexChar + X - 10;
 }
switch (Val % 10) {
       case 1: return "st";
       case 2: return "nd";
       case 3: return "rd";
       default: return "th";
     }
   }
 }
 
+/// PrintEscapedString - Print each character of the specified string, escaping
+/// it if it is not printable or if it is an escape char.
+void PrintEscapedString(StringRef Name, raw_ostream &Out);
+
 template <typename IteratorT>
 inline std::string join_impl(IteratorT Begin, IteratorT End,
                              StringRef Separator, std::input_iterator_tag) {
   std::string S;
   if (Begin == End)
     return S;
 
   S += (*Begin);

llvm/trunk/lib/IR/AsmWriter.cpp

static void PrintCallingConv(unsigned cc, raw_ostream &Out) {
   case CallingConv::AMDGPU_VS:     Out << "amdgpu_vs"; break;
   case CallingConv::AMDGPU_GS:     Out << "amdgpu_gs"; break;
   case CallingConv::AMDGPU_PS:     Out << "amdgpu_ps"; break;
   case CallingConv::AMDGPU_CS:     Out << "amdgpu_cs"; break;
   case CallingConv::AMDGPU_KERNEL: Out << "amdgpu_kernel"; break;
   }
 }
 
-// PrintEscapedString - Print each character of the specified string, escaping
-// it if it is not printable or if it is an escape char.
-static void PrintEscapedString(StringRef Name, raw_ostream &Out) {
+void llvm::PrintEscapedString(StringRef Name, raw_ostream &Out) {
   for (unsigned i = 0, e = Name.size(); i != e; ++i) {
     unsigned char C = Name[i];
     if (isprint(C) && C != '\\' && C != '"')
       Out << C;
     else
       Out << '\\' << hexdigit(C >> 4) << hexdigit(C & 0x0F);
   }
 }

llvm/trunk/lib/IR/Attributes.cpp

std::string Attribute::getAsString(bool InAttrGrp) const {
   //
   //   "kind"
   //   "kind" = "value"
   //
   if (isStringAttribute()) {
     std::string Result;
     Result += (Twine('"') + getKindAsString() + Twine('"')).str();
 
-    StringRef Val = pImpl->getValueAsString();
-    if (Val.empty()) return Result;
+    std::string AttrVal = pImpl->getValueAsString();
+    if (AttrVal.empty()) return Result;
 
-    Result += ("=\"" + Val + Twine('"')).str();
+    // Since some attribute strings contain special characters that cannot be
+    // printable, those have to be escaped to make the attribute value printable
+    // as is.  e.g. "\01__gnu_mcount_nc"
+    {
+      raw_string_ostream OS(Result);
+      OS << "=\"";
+      PrintEscapedString(AttrVal, OS);
+      OS << "\"";
+    }
     return Result;
   }
 
   llvm_unreachable("Unknown attribute");
 }
 
 bool Attribute::operator<(Attribute A) const {
   if (!pImpl && !A.pImpl) return false;