[HLSL][RootSignature] Correct `RootSignatureParser` to use correct `SourceLocation` in diagnostics #147084

inbelic · 2025-07-04T16:26:25Z

The SourceLocation of a RootSignatureToken is incorrectly set to be the "offset" into the concatenated string that denotes the rootsignature. This causes an issue when the StringLiteral is a multi-line expansion macro, since the offset will not account for the characters between StringLiteral tokens.

This pr resolves this by retaining the SourceLocation information that is kept in StringLiteral and then converting the offset in the concatenated string into the proper SourceLocation using the StringLiteral::getLocationOfByte interface. To do so, we will need to adjust the RootSignatureToken to only hold its offset into the root signature string. Then when the parser will use the token, it will need to compute its actual SourceLocation.

See linked issue for more context.

For example:

#define DemoRootSignature \
 "CBV(b0)," \
 "RootConstants(num32BitConstants = 3, b0, invalid)"
  expected caret location ---------------^
  actual caret location ------------^

The caret points 5 characters early because the current offset did not account for the characters:

'"' ' ' '\' ' ' '"'
 1   2   3   4   5

Updates RootSignatureParser to retain SourceLocation information by retaining the StringLiteral and passing the underlying StringRef to the Lexer
Updates RootSignatureLexer so that the constructed tokens only reflect an offset into the StringRef
Updates RootSignatureParser to directly construct its used Lexer so that the StringLiteral is directly tied with the string used in the RootSignatureLexer
Updates RootSignatureParser to use StringLiteral::getLocationOfByte to get the actual token location for diagnostics
Updates ParseHLSLRootSignatureTest to construct a phony AST/StringLiteral for the test cases
Adds a test to RootSignature-err.hlsl showing that the SourceLocation is correctly set for diagnostics in a multi-line macro expansion

Resolves: #146967

inbelic · 2025-07-04T16:28:20Z

clang/test/SemaHLSL/RootSignature-err.hlsl

+ "CBV(b0)," \
+ "RootConstants(num32BitConstants = 3, b0, invalid)"
+
+// CHECK: note: expanded from macro 'MultiLineRootSignature'


The generation of the note is checked using FileCheck because the expanded from macro note is generated by the preprocessor, which is not matchable in verify.

If I am missing something to make it work with verify that would be preferred!

Most preprocessor errors do match with the diagnostic verifier. These specifically are awkward because they come from the diagnostic renderer rather than being emitted directly. It looks like all the other tests for them are using FileCheck so that seems like the right approach.

We should probably include the specific line and column numbers in the matches. That makes the tests harder to update, but it's really the best way to make sure the source locations are pointing at the right places.

- the StringLiteral between the Lexer and Parser is now inherently coupled together and this constructor will enforce such

llvmbot · 2025-07-04T17:45:31Z

@llvm/pr-subscribers-hlsl

@llvm/pr-subscribers-clang

Author: Finn Plummer (inbelic)

Changes

The SourceLocation of a RootSignatureToken is incorrectly set to be the "offset" into the concatenated string that denotes the rootsignature. This causes an issue when the StringLiteral is a multi-line expansion macro as the offset will not account for the character between StringLiteral tokens.

This pr resolved by retaining the SourceLocation information that is kept in StringLiteral and then converting the offset in the concatenated string into the proper SourceLocation using the StringLiteral::getLocationOfByte interface. To do so, we will need to adjust the RootSignatureToken to only hold its offset into the root signature string. Then when the parser will use the token, it will need to compute its actual SourceLocation.

See linked issue for more context.

For example:

#define DemoRootSignature \
 "CBV(b0)," \
 "RootConstants(num32BitConstants = 3, b0, invalid)"
  expected caret location --------------------^
  actual caret location ------------------^

The caret points 5 characters early because the current offset did not account for the characters:

'"' ' ' '\' ' ' '"'
 1   2   3   4   5

Updates RootSignatureParser to retain SourceLocation information by retaining the StringLiteral and passing the underlying StringRef to the Lexer
Updates RootSignatureLexer so that the constructed tokens only reflect an offset into the StringRef
Updates RootSignatureParser to directly construct its used Lexer so that the StringLiteral is directly tied with the string used in the RootSignatureLexer
Updates RootSignatureParser to use StringLiteral::getLocationOfByte to get the actual token location for diagnostics
Updates ParseHLSLRootSignatureTest to construct a phony AST/StringLiteral for the test cases
Adds a test to RootSignature-err.hlsl showing that the SourceLocation is correctly set for diagnostics in a multi-line macro expansion

Resolves: #146967

Patch is 53.74 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/147084.diff

8 Files Affected:

(modified) clang/include/clang/Lex/LexHLSLRootSignature.h (+10-12)
(modified) clang/include/clang/Parse/ParseHLSLRootSignature.h (+10-3)
(modified) clang/lib/Lex/LexHLSLRootSignature.cpp (+3-3)
(modified) clang/lib/Parse/ParseDeclCXX.cpp (+3-7)
(modified) clang/lib/Parse/ParseHLSLRootSignature.cpp (+93-53)
(modified) clang/test/SemaHLSL/RootSignature-err.hlsl (+12)
(modified) clang/unittests/Lex/LexHLSLRootSignatureTest.cpp (+4-9)
(modified) clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp (+158-112)

diff --git a/clang/include/clang/Lex/LexHLSLRootSignature.h b/clang/include/clang/Lex/LexHLSLRootSignature.h
index 9901485b44d38..9bfefc96335b8 100644
--- a/clang/include/clang/Lex/LexHLSLRootSignature.h
+++ b/clang/include/clang/Lex/LexHLSLRootSignature.h
@@ -31,16 +31,17 @@ struct RootSignatureToken {
 
   Kind TokKind = Kind::invalid;
 
-  // Retain the SouceLocation of the token for diagnostics
-  clang::SourceLocation TokLoc;
+  // Retain the location offset of the token in the Signature
+  // string
+  uint32_t LocOffset;
 
   // Retain spelling of an numeric constant to be parsed later
   StringRef NumSpelling;
 
   // Constructors
-  RootSignatureToken(clang::SourceLocation TokLoc) : TokLoc(TokLoc) {}
-  RootSignatureToken(Kind TokKind, clang::SourceLocation TokLoc)
-      : TokKind(TokKind), TokLoc(TokLoc) {}
+  RootSignatureToken(uint32_t LocOffset) : LocOffset(LocOffset) {}
+  RootSignatureToken(Kind TokKind, uint32_t LocOffset)
+      : TokKind(TokKind), LocOffset(LocOffset) {}
 };
 
 inline const DiagnosticBuilder &
@@ -61,8 +62,7 @@ operator<<(const DiagnosticBuilder &DB, const RootSignatureToken::Kind Kind) {
 
 class RootSignatureLexer {
 public:
-  RootSignatureLexer(StringRef Signature, clang::SourceLocation SourceLoc)
-      : Buffer(Signature), SourceLoc(SourceLoc) {}
+  RootSignatureLexer(StringRef Signature) : Buffer(Signature) {}
 
   /// Consumes and returns the next token.
   RootSignatureToken consumeToken();
@@ -76,15 +76,13 @@ class RootSignatureLexer {
   }
 
 private:
-  // Internal buffer to iterate over
+  // Internal buffer state
   StringRef Buffer;
+  uint32_t LocOffset = 0;
 
   // Current peek state
   std::optional<RootSignatureToken> NextToken = std::nullopt;
 
-  // Passed down parameters from Sema
-  clang::SourceLocation SourceLoc;
-
   /// Consumes the buffer and returns the lexed token.
   RootSignatureToken lexToken();
 
@@ -92,7 +90,7 @@ class RootSignatureLexer {
   /// Updates the SourceLocation appropriately.
   void advanceBuffer(unsigned NumCharacters = 1) {
     Buffer = Buffer.drop_front(NumCharacters);
-    SourceLoc = SourceLoc.getLocWithOffset(NumCharacters);
+    LocOffset += NumCharacters;
   }
 };
 
diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h b/clang/include/clang/Parse/ParseHLSLRootSignature.h
index 66a5a3b7eaad0..b0ef617a13c28 100644
--- a/clang/include/clang/Parse/ParseHLSLRootSignature.h
+++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h
@@ -13,6 +13,7 @@
 #ifndef LLVM_CLANG_PARSE_PARSEHLSLROOTSIGNATURE_H
 #define LLVM_CLANG_PARSE_PARSEHLSLROOTSIGNATURE_H
 
+#include "clang/AST/Expr.h"
 #include "clang/Basic/DiagnosticParse.h"
 #include "clang/Lex/LexHLSLRootSignature.h"
 #include "clang/Lex/Preprocessor.h"
@@ -29,7 +30,7 @@ class RootSignatureParser {
 public:
   RootSignatureParser(llvm::dxbc::RootSignatureVersion Version,
                       SmallVector<llvm::hlsl::rootsig::RootElement> &Elements,
-                      RootSignatureLexer &Lexer, clang::Preprocessor &PP);
+                      StringLiteral *Signature, Preprocessor &PP);
 
   /// Consumes tokens from the Lexer and constructs the in-memory
   /// representations of the RootElements. Tokens are consumed until an
@@ -187,11 +188,17 @@ class RootSignatureParser {
   bool tryConsumeExpectedToken(RootSignatureToken::Kind Expected);
   bool tryConsumeExpectedToken(ArrayRef<RootSignatureToken::Kind> Expected);
 
+  /// Convert the token's offset in the signature string to its SourceLocation
+  ///
+  /// This allows to currently retrieve the location for multi-token
+  /// StringLiterals
+  SourceLocation getTokenLocation(RootSignatureToken Tok);
+
 private:
   llvm::dxbc::RootSignatureVersion Version;
   SmallVector<llvm::hlsl::rootsig::RootElement> &Elements;
-  RootSignatureLexer &Lexer;
-
+  clang::StringLiteral *Signature;
+  RootSignatureLexer Lexer;
   clang::Preprocessor &PP;
 
   RootSignatureToken CurToken;
diff --git a/clang/lib/Lex/LexHLSLRootSignature.cpp b/clang/lib/Lex/LexHLSLRootSignature.cpp
index e5de9ad15b07f..a89462c13c8e3 100644
--- a/clang/lib/Lex/LexHLSLRootSignature.cpp
+++ b/clang/lib/Lex/LexHLSLRootSignature.cpp
@@ -27,10 +27,10 @@ RootSignatureToken RootSignatureLexer::lexToken() {
   advanceBuffer(Buffer.take_while(isspace).size());
 
   if (isEndOfBuffer())
-    return RootSignatureToken(TokenKind::end_of_stream, SourceLoc);
+    return RootSignatureToken(TokenKind::end_of_stream, LocOffset);
 
   // Record where this token is in the text for usage in parser diagnostics
-  RootSignatureToken Result(SourceLoc);
+  RootSignatureToken Result(LocOffset);
 
   char C = Buffer.front();
 
@@ -62,7 +62,7 @@ RootSignatureToken RootSignatureLexer::lexToken() {
 
   // All following tokens require at least one additional character
   if (Buffer.size() <= 1) {
-    Result = RootSignatureToken(TokenKind::invalid, SourceLoc);
+    Result = RootSignatureToken(TokenKind::invalid, LocOffset);
     return Result;
   }
 
diff --git a/clang/lib/Parse/ParseDeclCXX.cpp b/clang/lib/Parse/ParseDeclCXX.cpp
index 6b0564dca6f45..d3bc6f1e89832 100644
--- a/clang/lib/Parse/ParseDeclCXX.cpp
+++ b/clang/lib/Parse/ParseDeclCXX.cpp
@@ -4944,20 +4944,16 @@ void Parser::ParseHLSLRootSignatureAttributeArgs(ParsedAttributes &Attrs) {
   }
 
   // Construct our identifier
-  StringRef Signature = StrLiteral.value()->getString();
+  StringLiteral *Signature = StrLiteral.value();
   auto [DeclIdent, Found] =
-      Actions.HLSL().ActOnStartRootSignatureDecl(Signature);
+      Actions.HLSL().ActOnStartRootSignatureDecl(Signature->getString());
   // If we haven't found an already defined DeclIdent then parse the root
   // signature string and construct the in-memory elements
   if (!Found) {
-    // Offset location 1 to account for '"'
-    SourceLocation SignatureLoc =
-        StrLiteral.value()->getExprLoc().getLocWithOffset(1);
     // Invoke the root signature parser to construct the in-memory constructs
-    hlsl::RootSignatureLexer Lexer(Signature, SignatureLoc);
     SmallVector<llvm::hlsl::rootsig::RootElement> RootElements;
     hlsl::RootSignatureParser Parser(getLangOpts().HLSLRootSigVer, RootElements,
-                                     Lexer, PP);
+                                     Signature, PP);
     if (Parser.parse()) {
       T.consumeClose();
       return;
diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp
index 96d3999ff2acb..ebaf7ba60fa17 100644
--- a/clang/lib/Parse/ParseHLSLRootSignature.cpp
+++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp
@@ -19,10 +19,10 @@ using TokenKind = RootSignatureToken::Kind;
 
 RootSignatureParser::RootSignatureParser(
     llvm::dxbc::RootSignatureVersion Version,
-    SmallVector<RootElement> &Elements, RootSignatureLexer &Lexer,
+    SmallVector<RootElement> &Elements, StringLiteral *Signature,
     Preprocessor &PP)
-    : Version(Version), Elements(Elements), Lexer(Lexer), PP(PP),
-      CurToken(SourceLocation()) {}
+    : Version(Version), Elements(Elements), Signature(Signature),
+      Lexer(Signature->getString()), PP(PP), CurToken(0) {}
 
 bool RootSignatureParser::parse() {
   // Iterate as many RootElements as possible
@@ -91,7 +91,8 @@ std::optional<llvm::dxbc::RootFlags> RootSignatureParser::parseRootFlags() {
   // Handle the edge-case of '0' to specify no flags set
   if (tryConsumeExpectedToken(TokenKind::int_literal)) {
     if (!verifyZeroFlag()) {
-      getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_non_zero_flag);
+      getDiags().Report(getTokenLocation(CurToken),
+                        diag::err_hlsl_rootsig_non_zero_flag);
       return std::nullopt;
     }
   } else {
@@ -141,7 +142,8 @@ std::optional<RootConstants> RootSignatureParser::parseRootConstants() {
 
   // Check mandatory parameters where provided
   if (!Params->Num32BitConstants.has_value()) {
-    getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_missing_param)
+    getDiags().Report(getTokenLocation(CurToken),
+                      diag::err_hlsl_rootsig_missing_param)
         << TokenKind::kw_num32BitConstants;
     return std::nullopt;
   }
@@ -149,7 +151,8 @@ std::optional<RootConstants> RootSignatureParser::parseRootConstants() {
   Constants.Num32BitConstants = Params->Num32BitConstants.value();
 
   if (!Params->Reg.has_value()) {
-    getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_missing_param)
+    getDiags().Report(getTokenLocation(CurToken),
+                      diag::err_hlsl_rootsig_missing_param)
         << TokenKind::bReg;
     return std::nullopt;
   }
@@ -209,7 +212,8 @@ std::optional<RootDescriptor> RootSignatureParser::parseRootDescriptor() {
 
   // Check mandatory parameters were provided
   if (!Params->Reg.has_value()) {
-    getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_missing_param)
+    getDiags().Report(getTokenLocation(CurToken),
+                      diag::err_hlsl_rootsig_missing_param)
         << ExpectedReg;
     return std::nullopt;
   }
@@ -258,7 +262,8 @@ std::optional<DescriptorTable> RootSignatureParser::parseDescriptorTable() {
 
     if (tryConsumeExpectedToken(TokenKind::kw_visibility)) {
       if (Visibility.has_value()) {
-        getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+        getDiags().Report(getTokenLocation(CurToken),
+                          diag::err_hlsl_rootsig_repeat_param)
             << CurToken.TokKind;
         return std::nullopt;
       }
@@ -328,7 +333,8 @@ RootSignatureParser::parseDescriptorTableClause() {
 
   // Check mandatory parameters were provided
   if (!Params->Reg.has_value()) {
-    getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_missing_param)
+    getDiags().Report(getTokenLocation(CurToken),
+                      diag::err_hlsl_rootsig_missing_param)
         << ExpectedReg;
     return std::nullopt;
   }
@@ -372,7 +378,8 @@ std::optional<StaticSampler> RootSignatureParser::parseStaticSampler() {
 
   // Check mandatory parameters were provided
   if (!Params->Reg.has_value()) {
-    getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_missing_param)
+    getDiags().Report(getTokenLocation(CurToken),
+                      diag::err_hlsl_rootsig_missing_param)
         << TokenKind::sReg;
     return std::nullopt;
   }
@@ -437,7 +444,8 @@ RootSignatureParser::parseRootConstantParams() {
     // `num32BitConstants` `=` POS_INT
     if (tryConsumeExpectedToken(TokenKind::kw_num32BitConstants)) {
       if (Params.Num32BitConstants.has_value()) {
-        getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+        getDiags().Report(getTokenLocation(CurToken),
+                          diag::err_hlsl_rootsig_repeat_param)
             << CurToken.TokKind;
         return std::nullopt;
       }
@@ -454,7 +462,8 @@ RootSignatureParser::parseRootConstantParams() {
     // `b` POS_INT
     if (tryConsumeExpectedToken(TokenKind::bReg)) {
       if (Params.Reg.has_value()) {
-        getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+        getDiags().Report(getTokenLocation(CurToken),
+                          diag::err_hlsl_rootsig_repeat_param)
             << CurToken.TokKind;
         return std::nullopt;
       }
@@ -467,7 +476,8 @@ RootSignatureParser::parseRootConstantParams() {
     // `space` `=` POS_INT
     if (tryConsumeExpectedToken(TokenKind::kw_space)) {
       if (Params.Space.has_value()) {
-        getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+        getDiags().Report(getTokenLocation(CurToken),
+                          diag::err_hlsl_rootsig_repeat_param)
             << CurToken.TokKind;
         return std::nullopt;
       }
@@ -484,7 +494,8 @@ RootSignatureParser::parseRootConstantParams() {
     // `visibility` `=` SHADER_VISIBILITY
     if (tryConsumeExpectedToken(TokenKind::kw_visibility)) {
       if (Params.Visibility.has_value()) {
-        getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+        getDiags().Report(getTokenLocation(CurToken),
+                          diag::err_hlsl_rootsig_repeat_param)
             << CurToken.TokKind;
         return std::nullopt;
       }
@@ -512,7 +523,8 @@ RootSignatureParser::parseRootDescriptorParams(TokenKind RegType) {
     // ( `b` | `t` | `u`) POS_INT
     if (tryConsumeExpectedToken(RegType)) {
       if (Params.Reg.has_value()) {
-        getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+        getDiags().Report(getTokenLocation(CurToken),
+                          diag::err_hlsl_rootsig_repeat_param)
             << CurToken.TokKind;
         return std::nullopt;
       }
@@ -525,7 +537,8 @@ RootSignatureParser::parseRootDescriptorParams(TokenKind RegType) {
     // `space` `=` POS_INT
     if (tryConsumeExpectedToken(TokenKind::kw_space)) {
       if (Params.Space.has_value()) {
-        getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+        getDiags().Report(getTokenLocation(CurToken),
+                          diag::err_hlsl_rootsig_repeat_param)
             << CurToken.TokKind;
         return std::nullopt;
       }
@@ -542,7 +555,8 @@ RootSignatureParser::parseRootDescriptorParams(TokenKind RegType) {
     // `visibility` `=` SHADER_VISIBILITY
     if (tryConsumeExpectedToken(TokenKind::kw_visibility)) {
       if (Params.Visibility.has_value()) {
-        getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+        getDiags().Report(getTokenLocation(CurToken),
+                          diag::err_hlsl_rootsig_repeat_param)
             << CurToken.TokKind;
         return std::nullopt;
       }
@@ -559,7 +573,8 @@ RootSignatureParser::parseRootDescriptorParams(TokenKind RegType) {
     // `flags` `=` ROOT_DESCRIPTOR_FLAGS
     if (tryConsumeExpectedToken(TokenKind::kw_flags)) {
       if (Params.Flags.has_value()) {
-        getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+        getDiags().Report(getTokenLocation(CurToken),
+                          diag::err_hlsl_rootsig_repeat_param)
             << CurToken.TokKind;
         return std::nullopt;
       }
@@ -587,7 +602,8 @@ RootSignatureParser::parseDescriptorTableClauseParams(TokenKind RegType) {
     // ( `b` | `t` | `u` | `s`) POS_INT
     if (tryConsumeExpectedToken(RegType)) {
       if (Params.Reg.has_value()) {
-        getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+        getDiags().Report(getTokenLocation(CurToken),
+                          diag::err_hlsl_rootsig_repeat_param)
             << CurToken.TokKind;
         return std::nullopt;
       }
@@ -600,7 +616,8 @@ RootSignatureParser::parseDescriptorTableClauseParams(TokenKind RegType) {
     // `numDescriptors` `=` POS_INT | unbounded
     if (tryConsumeExpectedToken(TokenKind::kw_numDescriptors)) {
       if (Params.NumDescriptors.has_value()) {
-        getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+        getDiags().Report(getTokenLocation(CurToken),
+                          diag::err_hlsl_rootsig_repeat_param)
             << CurToken.TokKind;
         return std::nullopt;
       }
@@ -623,7 +640,8 @@ RootSignatureParser::parseDescriptorTableClauseParams(TokenKind RegType) {
     // `space` `=` POS_INT
     if (tryConsumeExpectedToken(TokenKind::kw_space)) {
       if (Params.Space.has_value()) {
-        getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+        getDiags().Report(getTokenLocation(CurToken),
+                          diag::err_hlsl_rootsig_repeat_param)
             << CurToken.TokKind;
         return std::nullopt;
       }
@@ -640,7 +658,8 @@ RootSignatureParser::parseDescriptorTableClauseParams(TokenKind RegType) {
     // `offset` `=` POS_INT | DESCRIPTOR_RANGE_OFFSET_APPEND
     if (tryConsumeExpectedToken(TokenKind::kw_offset)) {
       if (Params.Offset.has_value()) {
-        getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+        getDiags().Report(getTokenLocation(CurToken),
+                          diag::err_hlsl_rootsig_repeat_param)
             << CurToken.TokKind;
         return std::nullopt;
       }
@@ -663,7 +682,8 @@ RootSignatureParser::parseDescriptorTableClauseParams(TokenKind RegType) {
     // `flags` `=` DESCRIPTOR_RANGE_FLAGS
     if (tryConsumeExpectedToken(TokenKind::kw_flags)) {
       if (Params.Flags.has_value()) {
-        getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+        getDiags().Report(getTokenLocation(CurToken),
+                          diag::err_hlsl_rootsig_repeat_param)
             << CurToken.TokKind;
         return std::nullopt;
       }
@@ -692,7 +712,8 @@ RootSignatureParser::parseStaticSamplerParams() {
     // `s` POS_INT
     if (tryConsumeExpectedToken(TokenKind::sReg)) {
       if (Params.Reg.has_value()) {
-        getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+        getDiags().Report(getTokenLocation(CurToken),
+                          diag::err_hlsl_rootsig_repeat_param)
             << CurToken.TokKind;
         return std::nullopt;
       }
@@ -705,7 +726,8 @@ RootSignatureParser::parseStaticSamplerParams() {
     // `filter` `=` FILTER
     if (tryConsumeExpectedToken(TokenKind::kw_filter)) {
       if (Params.Filter.has_value()) {
-        getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+        getDiags().Report(getTokenLocation(CurToken),
+                          diag::err_hlsl_rootsig_repeat_param)
             << CurToken.TokKind;
         return std::nullopt;
       }
@@ -722,7 +744,8 @@ RootSignatureParser::parseStaticSamplerParams() {
     // `addressU` `=` TEXTURE_ADDRESS
     if (tryConsumeExpectedToken(TokenKind::kw_addressU)) {
       if (Params.AddressU.has_value()) {
-        getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+        getDiags().Report(getTokenLocation(CurToken),
+                          diag::err_hlsl_rootsig_repeat_param)
             << CurToken.TokKind;
         return std::nullopt;
       }
@@ -739,7 +762,8 @@ RootSignatureParser::parseStaticSamplerParams() {
     // `addressV` `=` TEXTURE_ADDRESS
     if (tryConsumeExpectedToken(TokenKind::kw_addressV)) {
       if (Params.AddressV.has_value()) {
-        getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+        getDiags().Report(getTokenLocation(CurToken),
+                          diag::err_hlsl_rootsig_repeat_param)
             << CurToken.TokKind;
         return std::nullopt;
       }
@@ -756,7 +780,8 @@ RootSignatureParser::parseStaticSamplerParams() {
     // `addressW` `=` TEXTURE_ADDRESS
     if (tryConsumeExpectedToken(TokenKind::kw_addressW)) {
       if (Params.AddressW.has_value()) {
-        getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+        getDiags().Report(getTokenLocation(CurToken),
+                          diag::err_hlsl_rootsig_repeat_param)
             << CurToken.TokKind;
         return std::nullopt;
       }
@@ -773,7 +798,8 @@ RootSignatureParser::parseStaticSamplerParams() {
     // `mipLODBias` `=` NUMBER
     if (tryConsumeExpectedToken(TokenKind::kw_mipLODBias)) {
       if (Params.MipLODBias.has_value()) {
-        getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+        getDiags().Report(getTokenLocation(CurToken),
+                          diag::err_hlsl_rootsig_repeat_param)
             << CurToken.TokKind;
         return std::nullopt;
       }
@@ -790,7 +816,8 @@ RootSignatureParser::parseStaticSamplerParams() {
     // `maxAnisotropy` `=` POS_INT
     if (tryConsumeExpectedToken(TokenKind::kw_maxAnisotropy)) {
       if (Params.MaxAnisotropy.has_value()) {
-        getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+        getDiags().Report(getTokenLocation(CurToken),
+                          diag::err_hlsl_rootsig_repeat_param)
             << CurToken.TokKind;
         return std::nullopt;
       }
@@ -807,7 +834,8 @@ RootSignatureParser::parseStaticSamplerParams() {
     // `comparisonFunc` `=` COMPARISON_FUNC
     if (tryConsumeExpectedToken(TokenKind::kw_comparisonFunc)) {
       if (Params.CompFunc.has_value()) {
-        getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+        getDiags().Report(getTokenLocation(CurToken),
+        ...
[truncated]

llvm-beanz

A couple really small comments, but LGTM.

clang/include/clang/Parse/ParseHLSLRootSignature.h

llvm-beanz · 2025-07-07T18:32:55Z

clang/test/SemaHLSL/RootSignature-err.hlsl

+ "CBV(b0)," \
+ "RootConstants(num32BitConstants = 3, b0, invalid)"
+
+// CHECK: note: expanded from macro 'MultiLineRootSignature'


Most preprocessor errors do match with the diagnostic verifier. These specifically are awkward because they come from the diagnostic renderer rather than being emitted directly. It looks like all the other tests for them are using FileCheck so that seems like the right approach.

We should probably include the specific line and column numbers in the matches. That makes the tests harder to update, but it's really the best way to make sure the source locations are pointing at the right places.

clang/test/SemaHLSL/RootSignature-err.hlsl

inbelic commented Jul 4, 2025

View reviewed changes

inbelic force-pushed the inbelic/rs-fix-lexer branch from 28522fc to 10b172e Compare July 4, 2025 16:43

inbelic added 6 commits July 4, 2025 17:10

nfc: add phony ASTContext and StringLiteral to ParseHLSL

34d3879

nfc: add StringLiteral as a member of RootSignatureParser

d84d287

update RootSignatureToken to hold the offset into the signature string

9351946

nfc: instantiate Lexer in Parser constructor

dece8af

- the StringLiteral between the Lexer and Parser is now inherently coupled together and this constructor will enforce such

add sample testcase

b953140

rebase: clean-ups

1a64eff

inbelic force-pushed the inbelic/rs-fix-lexer branch from 10b172e to 1a64eff Compare July 4, 2025 17:44

inbelic marked this pull request as ready for review July 4, 2025 17:45

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" HLSL HLSL Language Support labels Jul 4, 2025

llvm-beanz approved these changes Jul 7, 2025

View reviewed changes

inbelic added 4 commits July 7, 2025 20:59

review: remove unneeded namespacing

7da53d2

review: improve flag

0132e74

review: specify column value as well

d4cf62d

nfc, self-review: simplify diag reporting with helper function

29cde65

bogner approved these changes Jul 8, 2025

View reviewed changes

inbelic merged commit 3dec46d into llvm:main Jul 8, 2025
10 checks passed

inbelic deleted the inbelic/rs-fix-lexer branch July 14, 2025 17:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[HLSL][RootSignature] Correct `RootSignatureParser` to use correct `SourceLocation` in diagnostics #147084

[HLSL][RootSignature] Correct `RootSignatureParser` to use correct `SourceLocation` in diagnostics #147084

Uh oh!

inbelic commented Jul 4, 2025 •

edited

Loading

Uh oh!

inbelic Jul 4, 2025 •

edited

Loading

Uh oh!

llvm-beanz Jul 7, 2025

Uh oh!

llvmbot commented Jul 4, 2025 •

edited

Loading

Uh oh!

llvm-beanz left a comment

Uh oh!

Uh oh!

llvm-beanz Jul 7, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[HLSL][RootSignature] Correct RootSignatureParser to use correct SourceLocation in diagnostics #147084

[HLSL][RootSignature] Correct RootSignatureParser to use correct SourceLocation in diagnostics #147084

Uh oh!

Conversation

inbelic commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

inbelic Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

llvm-beanz Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

llvmbot commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvm-beanz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvm-beanz Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[HLSL][RootSignature] Correct `RootSignatureParser` to use correct `SourceLocation` in diagnostics #147084

[HLSL][RootSignature] Correct `RootSignatureParser` to use correct `SourceLocation` in diagnostics #147084

inbelic commented Jul 4, 2025 •

edited

Loading

inbelic Jul 4, 2025 •

edited

Loading

llvmbot commented Jul 4, 2025 •

edited

Loading