[libclang] Fix conversion from `StringRef` to `CXString`

[egorzhdan]

Summary

CXString createRef(StringRef String) used to return an invalid string when invoked with some empty strings:

If a StringRef holds a non-nullptr pointer, for instance, pointing into contents of a larger string, and has a zero length, createRef previously returned the entire larger string, ignoring the fact that the actual string passed to it as a param is empty.

This was discovered when invoking c-index-test to dump the contents of documentation comments, in case the comment contains an empty HTML attribute, such as src="".

[egorzhdan]

XFAIL a test that was accidentally passing because of incorrect behavior

[egorzhdan]

clang/test/Index/comment-lots-of-unknown-commands.c

3

This test was never properly passing. Because of the bug in string conversion, the printed comments contained the entire source file and not just the comments' text, which was enough to cause // CHECK-s in the test to succeed.

// CHECK:         (CXComment_InlineCommand CommandName=[tel] RenderNormal HasTrailingNewline)
// CHECK:         (CXComment_InlineCommand CommandName=[n] RenderNormal HasTrailingNewline))
// CHECK:       (CXComment_VerbatimLine Text=[\n@Lo\n@il\n@tle\n@axt\n@ba\n@ust\n@ac\n@tpe\n@tpl\n@ctG\n@ru\n@m\n@tG\n@it\n@rh\n@G\n@rpc\n@el\n@er\n@w\n@eo\n@tx\n@oo\n@dD\n@dD\n*/\nvoid f();\n\n// CHECK:  CommentAST=[\n// CHECK:    (CXComment_FullComment\n// CHECK:       (CXComment_Paragraph\n// CHECK:      ...

[gribozavr2]

clang/test/Index/comment-lots-of-unknown-commands.c

3

Please update the test to pass then. Here's the diff:

diff --git a/clang/test/Index/comment-lots-of-unknown-commands.c b/clang/test/Index/comment-lots-of-unknown-commands.c
index 41a03d394488..e1adcc150b1e 100644
--- a/clang/test/Index/comment-lots-of-unknown-commands.c
+++ b/clang/test/Index/comment-lots-of-unknown-commands.c
@@ -1,6 +1,5 @@
 // RUN: c-index-test -test-load-source-reparse 1 local %s | FileCheck %s

-// XFAIL: *

 // See PR 21254. We had too few bits to encode command IDs so if you created
 // enough of them the ID codes would wrap around. This test creates commands up
@@ -183,7 +182,7 @@ void f();
 // CHECK:         (CXComment_InlineCommand CommandName=[ei] RenderNormal HasTrailingNewline)
 // CHECK:         (CXComment_InlineCommand CommandName=[oun] RenderNormal HasTrailingNewline)
 // CHECK:         (CXComment_InlineCommand CommandName=[ou] RenderNormal HasTrailingNewline)
-// CHECK:         (CXComment_InlineCommand CommandName=[nl] RenderNormal HasTrailingNewline)
+// CHECK:         (CXComment_InlineCommand CommandName=[n] RenderNormal HasTrailingNewline)
 // CHECK:         (CXComment_InlineCommand CommandName=[ien] RenderNormal HasTrailingNewline)
 // CHECK:         (CXComment_InlineCommand CommandName=[fr] RenderNormal HasTrailingNewline)
 // CHECK:         (CXComment_InlineCommand CommandName=[en] RenderNormal HasTrailingNewline)
@@ -204,7 +203,7 @@ void f();
 // CHECK:         (CXComment_InlineCommand CommandName=[fro] RenderNormal HasTrailingNewline)
 // CHECK:         (CXComment_InlineCommand CommandName=[ast] RenderNormal HasTrailingNewline)
 // CHECK:         (CXComment_InlineCommand CommandName=[ae] RenderNormal HasTrailingNewline)
-// CHECK:         (CXComment_InlineCommand CommandName=[nN] RenderNormal HasTrailingNewline)
+// CHECK:         (CXComment_InlineCommand CommandName=[n] RenderNormal HasTrailingNewline)
 // CHECK:         (CXComment_InlineCommand CommandName=[pc] RenderNormal HasTrailingNewline)
 // CHECK:         (CXComment_InlineCommand CommandName=[tae] RenderNormal HasTrailingNewline)
 // CHECK:         (CXComment_InlineCommand CommandName=[ws] RenderNormal HasTrailingNewline)
@@ -268,10 +267,8 @@ void f();
 // CHECK:         (CXComment_InlineCommand CommandName=[an] RenderNormal HasTrailingNewline)
 // CHECK:         (CXComment_InlineCommand CommandName=[de] RenderNormal HasTrailingNewline)
 // CHECK:         (CXComment_InlineCommand CommandName=[tel] RenderNormal HasTrailingNewline)
-// CHECK:         (CXComment_InlineCommand CommandName=[nd] RenderNormal HasTrailingNewline)
-// CHECK:         (CXComment_InlineCommand CommandName=[dic] RenderNormal HasTrailingNewline)
+// CHECK:         (CXComment_InlineCommand CommandName=[n] RenderNormal HasTrailingNewline)
 // CHECK:         (CXComment_InlineCommand CommandName=[Lo] RenderNormal HasTrailingNewline)
-// CHECK:         (CXComment_InlineCommand CommandName=[il] RenderNormal HasTrailingNewline)
 // CHECK:         (CXComment_InlineCommand CommandName=[tle] RenderNormal HasTrailingNewline)
 // CHECK:         (CXComment_InlineCommand CommandName=[axt] RenderNormal HasTrailingNewline)
 // CHECK:         (CXComment_InlineCommand CommandName=[ba] RenderNormal HasTrailingNewline)
@@ -283,7 +280,6 @@ void f();
 // CHECK:         (CXComment_InlineCommand CommandName=[ru] RenderNormal HasTrailingNewline)
 // CHECK:         (CXComment_InlineCommand CommandName=[m] RenderNormal HasTrailingNewline)
 // CHECK:         (CXComment_InlineCommand CommandName=[tG] RenderNormal HasTrailingNewline)
-// CHECK:         (CXComment_InlineCommand CommandName=[it] RenderNormal HasTrailingNewline)
 // CHECK:         (CXComment_InlineCommand CommandName=[rh] RenderNormal HasTrailingNewline)
 // CHECK:         (CXComment_InlineCommand CommandName=[G] RenderNormal HasTrailingNewline)
 // CHECK:         (CXComment_InlineCommand CommandName=[rpc] RenderNormal HasTrailingNewline)

[egorzhdan]

clang/test/Index/comment-lots-of-unknown-commands.c

3

This doesn't look like the correct output though. I think the CommandName fields in the output should match the @-tags in the input (e.g. @s -> (CXComment_InlineCommand CommandName=[s] RenderNormal HasTrailingNewline)).

This looks like a bug in the logic this test is aiming to verify, not a mistake in the test itself. I unfortunately don't have enough context to fix the actual issue here, so I disabled the test instead. I'll also file an GitHub issue for this.

[aaronpuchert]

clang/test/Index/comment-lots-of-unknown-commands.c

3

They don't always match, because typo correction will change commands that are close enough to an existing command. That's why e.g. @nl shows up as @n in the AST. I remember that I had to change this test in D111190 because introducing the new commands made the typo correction kick in for some of these.

The proper fix for the test (see the comment below) should be to change the commands here to differ enough from existing commands again. The corrections to @n are likely caused by D125429, while @dic → @dir, @il/@it → @if come from D111190.

I don't think we're going to gain additional commands, but we might also make this "future-proof" by simply using long-enough sequences of random letters, let's say 4 to have a reasonable number.

clang/tools/libclang/CXString.cpp

81–82

Does this actually happen or are we just doing this to be safe?

[egorzhdan]

clang/test/Index/comment-lots-of-unknown-commands.c

3

Thanks @aaronpuchert! I didn't realize this is caused by typo correction. I'll put up a patch to re-enable the test.

clang/tools/libclang/CXString.cpp

81–82

It's possible to get an empty StringRef with a nullptr data here. I initially skipped this if and got a test failure because of it: instead of returning a null CXString, this method was returning an empty CXString.

clang/test/Index/comment-lots-of-unknown-commands.c

 // RUN: c-index-test -test-load-source-reparse 1 local %s | FileCheck %s
 
+// XFAIL: *
+
 // See PR 21254. We had too few bits to encode command IDs so if you created
 // enough of them the ID codes would wrap around. This test creates commands up
 // to an ID of 258. Ideally we should check for large numbers, but that would
 // require a test source file which is megabytes in size. This is the test case
 // from the PR.
 
 /**
 @s

clang/test/Index/comment-to-html-xml-conversion.cpp

 
 // CHECK: comment-to-html-xml-conversion.cpp:[[@LINE-2]]:6: FunctionDecl=comment_to_html_conversion_37:{{.*}} FullCommentAsHTML=[<p class="para-brief"> <span id="A"></span></p>] FullCommentAsXML=[<Function file="{{[^"]+}}comment-to-html-xml-conversion.cpp" line="[[@LINE-2]]" column="6"><Name>comment_to_html_conversion_37</Name><USR>c:@F@comment_to_html_conversion_37#</USR><Declaration>void comment_to_html_conversion_37()</Declaration><Abstract><Para> <anchor id="A"></anchor></Para></Abstract></Function>]
 // CHECK-NEXT:  CommentAST=[
 // CHECK-NEXT:    (CXComment_FullComment
 // CHECK-NEXT:       (CXComment_Paragraph
 // CHECK-NEXT:         (CXComment_Text Text=[ ] IsWhitespace)
 // CHECK-NEXT:         (CXComment_InlineCommand CommandName=[anchor] RenderAnchor Arg[0]=A)))]
 
+/// Aaa ccc<img src="">
+void comment_to_html_conversion_40();
+
+// CHECK: comment-to-html-xml-conversion.cpp:[[@LINE-2]]:6: FunctionDecl=comment_to_html_conversion_40:{{.*}} FullCommentAsHTML=[<p class="para-brief"> Aaa ccc<img src></p>] FullCommentAsXML=[<Function file="{{[^"]+}}comment-to-html-xml-conversion.cpp" line="[[@LINE-2]]" column="6"><Name>comment_to_html_conversion_40</Name><USR>c:@F@comment_to_html_conversion_40#</USR><Declaration>void comment_to_html_conversion_40()</Declaration><Abstract><Para> Aaa ccc<rawHTML><![CDATA[<img src>]]></rawHTML></Para></Abstract></Function>]
+// CHECK-NEXT:  CommentAST=[
+// CHECK-NEXT:    (CXComment_FullComment
+// CHECK-NEXT:       (CXComment_Paragraph
+// CHECK-NEXT:         (CXComment_Text Text=[ Aaa ccc])
+// CHECK-NEXT:         (CXComment_HTMLStartTag Name=[img] Attrs: src=)
 
 /// Aaa.
 class comment_to_xml_conversion_01 {
 // CHECK: comment-to-html-xml-conversion.cpp:[[@LINE-1]]:7: ClassDecl=comment_to_xml_conversion_01:{{.*}} FullCommentAsXML=[<Class file="{{[^"]+}}comment-to-html-xml-conversion.cpp" line="[[@LINE-1]]" column="7"><Name>comment_to_xml_conversion_01</Name><USR>c:@S@comment_to_xml_conversion_01</USR><Declaration>class comment_to_xml_conversion_01 {}</Declaration><Abstract><Para> Aaa.</Para></Abstract></Class>]
 
   /// \param aaa Blah blah.
   comment_to_xml_conversion_01(int aaa);
 

clang/tools/libclang/CXString.cpp

CXString createDup(const char *String) {
 
   CXString Str;
   Str.data = strdup(String);
   Str.private_flags = CXS_Malloc;
   return Str;
 }
 
 CXString createRef(StringRef String) {
+  if (!String.data())
+    return createNull();
+
+  // If the string is empty, it might point to a position in another string
+  // while having zero length. Make sure we don't create a reference to the
+  // larger string.
+  if (String.empty())
+    return createEmpty();
+
   // If the string is not nul-terminated, we have to make a copy.
 
   // FIXME: This is doing a one past end read, and should be removed! For memory
   // we don't manage, the API string can become unterminated at any time outside
   // our control.
 
-  if (!String.empty() && String.data()[String.size()] != 0)
+  if (String.data()[String.size()] != 0)
     return createDup(String);
 
   CXString Result;
   Result.data = String.data();
   Result.private_flags = (unsigned) CXS_Unmanaged;
   return Result;
 }