erraneous html with -emit-html

[llvmbot]

Bugzilla Link	2386
Resolution	FIXED
Resolved on	Mar 12, 2010 00:55
Version	unspecified
OS	Linux
Blocks	llvm/llvm-bugzilla-archive#2736 llvm/llvm-bugzilla-archive#3114
Attachments	the source code, the emitted html, how it looks like in the browser
Reporter	LLVM Bugzilla Contributor
CC	@efriedma-quic,@tkremenek

Extended Description

Some instances of (for indented text) are emitted as tr>. I've attached:

• the source-code file
• a snapshot on how it looks on Konqueror (it looks the same on Firefox)
• the emitted HTML file

This happened with LLVM SVN 51761 and CFE SVN 51761.

[efriedma-quic]

Yeah, definitely broken; I think there's a bug in RewriteBuffer::InsertText with InsertAfter = false; the code doesn't seem to distinguish between inserting text before and after properly. Not really familiar with it, though.

[lattner]

reduced testcase
Here's a reduced testcase, it looks like a bad interaction between AddLineNumbers and EscapeText in HTMLPrint. The bug still occurs when the other stuff in HTMLPrint is disabled. I'll take a look tomorrow.

[lattner]

Here is a patch:

--- lib/Rewrite/Rewriter.cpp (revision 52080)
+++ lib/Rewrite/Rewriter.cpp (working copy)
@@ -52,7 +52,8 @@
/// operation.
void RewriteBuffer::ReplaceText(unsigned OrigOffset, unsigned OrigLength,
const char *NewStr, unsigned NewLength) {

• unsigned RealOffset = getMappedOffset(OrigOffset, false);

• unsigned RealOffset = getMappedOffset(OrigOffset, true
• );
  Buffer.erase(RealOffset, OrigLength);
  Buffer.insert(RealOffset, NewStr, NewStr+NewLength);
  if (OrigLength != NewLength)

This was broken by r51514, which is the opposite of this patch. I will need to investigate which is the right fix. In the meantime this should be a workaround.

[lattner]

Here's a patch to clang that makes reduces the number of rewriter requests:

+++ Driver/HTMLPrint.cpp (working copy)
@@ -60,14 +60,14 @@
// Format the file.
unsigned FileID = R.getSourceMgr().getMainFileID();
html::AddLineNumbers(R, FileID);

• html::AddHeaderFooterInternalBuiltinCSS(R, FileID);

• //html::AddHeaderFooterInternalBuiltinCSS(R, FileID);

  // If we have a preprocessor, relex the file and syntax highlight.
  // We might not have a preprocessor if we come from a deserialized AST file,
  // for example.

• if (PP) html::SyntaxHighlight(R, FileID, *PP);
• if (PPF) html::HighlightMacros(R, FileID, *PP);

• //if (PP) html::SyntaxHighlight(R, FileID, *PP);

• //if (PPF) html::HighlightMacros(R, FileID, *PP);
  html::EscapeText(R, FileID, false, true);

  // Open the output.

[lattner]

... the last patch is useful when debugging this problem, I wouldn't suggest actually applying it :)

[tkremenek]

HTML output from scan-build on test suite case
The clang test suite now contains another test case of this bug:

$ cd test/Analysis
$ scan-build -V gcc -fsyntax-only CFNumber.c

Both bug reports don't have a problem where '&' is escaped and also within an inserted tag.

[lattner]

*** Bug llvm/llvm-bugzilla-archive#2736 has been marked as a duplicate of this bug. ***

[llvmbot]

The problem occurs when escaping Tabs. So, you won't have problems when you only use spaces in your source code...
If you set escape tabs to false (patch below) the output is correct. I'm not sure why the EscapeTabs is true here: it is supposed to replace tabs by space, but both won't be preserved in the HTML (so what's the difference?).
Note: the tab-escaping for the lines (whose spaces will be preserved) is somewhere else.

===================================================================
--- Driver/HTMLPrint.cpp (revision 66791)
+++ Driver/HTMLPrint.cpp (working copy)
@@ -72,7 +72,7 @@

if (PP) html::SyntaxHighlight(R, FID, *PP);
if (PPF) html::HighlightMacros(R, FID, *PP);

• html::EscapeText(R, FID, false, true);

• html::EscapeText(R, FID, false, false);

  // Open the output.
  FILE *OutputFILE;
  ===================================================================

I am not sure what is broken in the EscapeText method, but setting the original length (for ReplaceText) of a Tab to 0 will cure the problem, too.
Note: this patch is not supposed to be applied ..., just for illustration of the problem

--- lib/Rewrite/HTMLRewrite.cpp (revision 66791)
+++ lib/Rewrite/HTMLRewrite.cpp (working copy)
@@ -136,7 +136,7 @@
RB.ReplaceText(FilePos, 1, "     "
"   ", 6*NumSpaces);
else

•    RB.ReplaceText(FilePos, 1, "        ", NumSpaces);
  

•    RB.ReplaceText(FilePos, 0, "        ", NumSpaces);
   ColNo += NumSpaces;
   break;
  

  }
  ===================================================================

[lattner]

Ted are you ok with disabling tabs in the HTML output of the checker?

[tkremenek]

Ted are you ok with disabling tabs in the HTML output of the checker?

Not certain what you mean. The point of rewriting the tabs to spaces is to not have tabs in the HTML output (i.e., I don't know what "disabling tabs" means).

I find it interesting that changing:

•    RB.ReplaceText(FilePos, 1, "        ", NumSpaces);
  

to

•    RB.ReplaceText(FilePos, 0, "        ", NumSpaces);
  

"fixes" the problem. I suppose that means that the original '\t' character is not removed (and simply ignored by the browser)?

[tkremenek]

The problem occurs when escaping Tabs. So, you won't have problems when you
only use spaces in your source code...
If you set escape tabs to false (patch below) the output is correct. I'm not
sure why the EscapeTabs is true here: it is supposed to replace tabs by space,
but both won't be preserved in the HTML (so what's the difference?).

The difference is that the tabs appear in text that will be placed within a , so the inserted spaces will be honored by the browser.

[lattner]

right, because they end up in 's, I don't think we have to manually escape them. Just disabling this might be a simple way to work around the bug.

[tkremenek]

right, because they end up in 's, I don't think we have to manually escape
them. Just disabling this might be a simple way to work around the bug.

I did an experiment: only   is honored as forced whitespace, even with . All other whitespace, including regular spaces and tabs, are converted to a single whitespace character.

Consequently, if we want to get the indentation right in the syntax highlighted code, we need to replace \t with the appropriate number of  

[tkremenek]

right, because they end up in 's, I don't think we have to manually escape
them. Just disabling this might be a simple way to work around the bug.

I did an experiment: only   is honored as forced whitespace, even with
. All other whitespace, including regular spaces and tabs, are converted
to a single whitespace character.

That should have been "even within "

[lattner]

aha, so we are trying to change \t into some number of spaces, then change the spaces into  's? Perhaps inserting the right number of  's directly would work better. The rewriter doesn't like rewriting rewritten code.