[DataLayout] Refactor the rest of parseSpecification (llvm#104545)

The aim is to improve test coverage of data layout string parsing.

Pull Request: llvm#104545

Author: s-barannikov

lld/test/ELF/exclude-libs.s

@@ -3,7 +3,7 @@
 // RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
 // RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux \
 // RUN:   %p/Inputs/exclude-libs.s -o %t2.o
-// RUN: llvm-as --data-layout=elf %p/Inputs/exclude-libs.ll -o %t3.o
+// RUN: llvm-as --data-layout=e %p/Inputs/exclude-libs.ll -o %t3.o
 // RUN: mkdir -p %t.dir
 // RUN: rm -f %t.dir/exc.a
 // RUN: llvm-ar rcs %t.dir/exc.a %t2.o %t3.o

llvm/lib/IR/DataLayout.cpp

@@ -268,10 +268,6 @@ Expected<DataLayout> DataLayout::parse(StringRef LayoutString) {
   return Layout;
 }
 
-static Error reportError(const Twine &Message) {
-  return createStringError(inconvertibleErrorCode(), Message);
-}
-
 static Error createSpecFormatError(Twine Format) {
   return createStringError("malformed specification, must be of the form \"" +
                            Format + "\"");
@@ -336,46 +332,6 @@ static Error parseAlignment(StringRef Str, Align &Alignment, StringRef Name,
   return Error::success();
 }
 
-/// Checked version of split, to ensure mandatory subparts.
-static Error split(StringRef Str, char Separator,
-                   std::pair<StringRef, StringRef> &Split) {
-  assert(!Str.empty() && "parse error, string can't be empty here");
-  Split = Str.split(Separator);
-  if (Split.second.empty() && Split.first != Str)
-    return reportError("Trailing separator in datalayout string");
-  if (!Split.second.empty() && Split.first.empty())
-    return reportError("Expected token before separator in datalayout string");
-  return Error::success();
-}
-
-/// Get an unsigned integer, including error checks.
-template <typename IntTy> static Error getInt(StringRef R, IntTy &Result) {
-  bool error = R.getAsInteger(10, Result); (void)error;
-  if (error)
-    return reportError("not a number, or does not fit in an unsigned int");
-  return Error::success();
-}
-
-/// Get an unsigned integer representing the number of bits and convert it into
-/// bytes. Error out of not a byte width multiple.
-template <typename IntTy>
-static Error getIntInBytes(StringRef R, IntTy &Result) {
-  if (Error Err = getInt<IntTy>(R, Result))
-    return Err;
-  if (Result % 8)
-    return reportError("number of bits must be a byte width multiple");
-  Result /= 8;
-  return Error::success();
-}
-
-static Error getAddrSpace(StringRef R, unsigned &AddrSpace) {
-  if (Error Err = getInt(R, AddrSpace))
-    return Err;
-  if (!isUInt<24>(AddrSpace))
-    return reportError("Invalid address space, must be a 24-bit integer");
-  return Error::success();
-}
-
 Error DataLayout::parsePrimitiveSpec(StringRef Spec) {
   // [ifv]<size>:<abi>[:<pref>]
   SmallVector<StringRef, 3> Components;
@@ -536,100 +492,90 @@ Error DataLayout::parseSpecification(StringRef Spec) {
   if (Specifier == 'p')
     return parsePointerSpec(Spec);
 
-  // Split at ':'.
-  std::pair<StringRef, StringRef> Split;
-  if (Error Err = ::split(Spec, ':', Split))
-    return Err;
-
-  // Aliases used below.
-  StringRef &Tok = Split.first;   // Current token.
-  StringRef &Rest = Split.second; // The rest of the string.
-
-  char SpecifierChar = Tok.front();
-  Tok = Tok.substr(1);
-
-  switch (SpecifierChar) {
+  StringRef Rest = Spec.drop_front();
+  switch (Specifier) {
   case 's':
     // Deprecated, but ignoring here to preserve loading older textual llvm
     // ASM file
     break;
-  case 'E':
-    BigEndian = true;
-    break;
   case 'e':
-    BigEndian = false;
+  case 'E':
+    if (!Rest.empty())
+      return createStringError(
+          "malformed specification, must be just 'e' or 'E'");
+    BigEndian = Specifier == 'E';
     break;
   case 'n': // Native integer types.
-    while (true) {
-      unsigned Width;
-      if (Error Err = getInt(Tok, Width))
-        return Err;
-      if (Width == 0)
-        return reportError(
-            "Zero width native integer type in datalayout string");
-      LegalIntWidths.push_back(Width);
-      if (Rest.empty())
-        break;
-      if (Error Err = ::split(Rest, ':', Split))
+    // n<size>[:<size>]...
+    for (StringRef Str : split(Rest, ':')) {
+      unsigned BitWidth;
+      if (Error Err = parseSize(Str, BitWidth))
         return Err;
+      LegalIntWidths.push_back(BitWidth);
     }
     break;
   case 'S': { // Stack natural alignment.
-    uint64_t Alignment;
-    if (Error Err = getIntInBytes(Tok, Alignment))
+    // S<size>
+    if (Rest.empty())
+      return createSpecFormatError("S<size>");
+    Align Alignment;
+    if (Error Err = parseAlignment(Rest, Alignment, "stack natural"))
       return Err;
-    if (Alignment != 0 && !llvm::isPowerOf2_64(Alignment))
-      return reportError("Alignment is neither 0 nor a power of 2");
-    StackNaturalAlign = MaybeAlign(Alignment);
+    StackNaturalAlign = Alignment;
     break;
   }
   case 'F': {
-    switch (Tok.front()) {
+    // F<type><abi>
+    if (Rest.empty())
+      return createSpecFormatError("F<type><abi>");
+    char Type = Rest.front();
+    Rest = Rest.drop_front();
+    switch (Type) {
     case 'i':
       TheFunctionPtrAlignType = FunctionPtrAlignType::Independent;
       break;
     case 'n':
       TheFunctionPtrAlignType = FunctionPtrAlignType::MultipleOfFunctionAlign;
       break;
     default:
-      return reportError("Unknown function pointer alignment type in "
-                         "datalayout string");
+      return createStringError("unknown function pointer alignment type '" +
+                               Twine(Type) + "'");
     }
-    Tok = Tok.substr(1);
-    uint64_t Alignment;
-    if (Error Err = getIntInBytes(Tok, Alignment))
+    Align Alignment;
+    if (Error Err = parseAlignment(Rest, Alignment, "ABI"))
       return Err;
-    if (Alignment != 0 && !llvm::isPowerOf2_64(Alignment))
-      return reportError("Alignment is neither 0 nor a power of 2");
-    FunctionPtrAlign = MaybeAlign(Alignment);
+    FunctionPtrAlign = Alignment;
     break;
   }
   case 'P': { // Function address space.
-    if (Error Err = getAddrSpace(Tok, ProgramAddrSpace))
+    if (Rest.empty())
+      return createSpecFormatError("P<address space>");
+    if (Error Err = parseAddrSpace(Rest, ProgramAddrSpace))
       return Err;
     break;
   }
   case 'A': { // Default stack/alloca address space.
-    if (Error Err = getAddrSpace(Tok, AllocaAddrSpace))
+    if (Rest.empty())
+      return createSpecFormatError("A<address space>");
+    if (Error Err = parseAddrSpace(Rest, AllocaAddrSpace))
       return Err;
     break;
   }
   case 'G': { // Default address space for global variables.
-    if (Error Err = getAddrSpace(Tok, DefaultGlobalsAddrSpace))
+    if (Rest.empty())
+      return createSpecFormatError("G<address space>");
+    if (Error Err = parseAddrSpace(Rest, DefaultGlobalsAddrSpace))
       return Err;
     break;
   }
   case 'm':
-    if (!Tok.empty())
-      return reportError("Unexpected trailing characters after mangling "
-                         "specifier in datalayout string");
-    if (Rest.empty())
-      return reportError("Expected mangling specifier in datalayout string");
+    if (!Rest.consume_front(":") || Rest.empty())
+      return createSpecFormatError("m:<mangling>");
     if (Rest.size() > 1)
-      return reportError("Unknown mangling specifier in datalayout string");
+      return createStringError("unknown mangling mode");
     switch (Rest[0]) {
     default:
-      return reportError("Unknown mangling in datalayout string");
+      return createStringError("unknown mangling mode");
     case 'e':
       ManglingMode = MM_ELF;
       break;
@@ -654,7 +600,7 @@ Error DataLayout::parseSpecification(StringRef Spec) {
     }
     break;
   default:
-    return reportError("Unknown specifier in datalayout string");
+    return createStringError("unknown specifier '" + Twine(Specifier) + "'");
   }
 
   return Error::success();

llvm/test/Assembler/invalid-datalayout-override.ll

@@ -4,4 +4,4 @@
 ; RUN: llvm-as -data-layout "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128" < %s
 
 target datalayout = "A16777216"
-; CHECK: Invalid address space, must be a 24-bit integer
+; CHECK: address space must be a 24-bit integer

llvm/test/Bitcode/function-default-address-spaces.ll

@@ -1,7 +1,7 @@
 ; RUN: llvm-as %s  -o - | llvm-dis - | FileCheck %s -check-prefixes CHECK,PROG-AS0
 ; RUN: llvm-as -data-layout "P200" %s  -o - | llvm-dis | FileCheck %s -check-prefixes CHECK,PROG-AS200
 ; RUN: not llvm-as -data-layout "P123456789" %s -o /dev/null 2>&1 | FileCheck %s -check-prefix BAD-DATALAYOUT
-; BAD-DATALAYOUT: error: Invalid address space, must be a 24-bit integer
+; BAD-DATALAYOUT: error: address space must be a 24-bit integer
 
 ; PROG-AS0-NOT: target datalayout
 ; PROG-AS200: target datalayout = "P200"

llvm/test/Bitcode/invalid-functionptr-align.ll

@@ -0,0 +1,5 @@
+; Bitcode with invalid natural stack alignment.
+
+; RUN: not llvm-dis %s.bc -o - 2>&1 | FileCheck %s
+
+CHECK: error: stack natural alignment must be a power of two times the byte width

llvm/test/Bitcode/invalid-stack-align.ll

@@ -1,7 +1,7 @@
 ; RUN: llc -verify-machineinstrs -O0 -mcpu=pwr7 -filetype=obj %s -o - | \
 ; RUN: llvm-readobj --symbols - | FileCheck %s
 
-target datalayout = "E-p:64:64:64-S0-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f16:16:16-f32:32:32-f64:64:64-f128:128:128-v64:64:64-v128:128:128-a0:0:64-n32:64"
+target datalayout = "E-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f16:16:16-f32:32:32-f64:64:64-f128:128:128-v64:64:64-v128:128:128-a0:0:64-n32:64"
 target triple = "powerpc64-unknown-linux-gnu"
 
 @nextIdx = external thread_local global i32

…/Bitcode/invalid-functionptr-align.ll.bc …m/test/Bitcode/invalid-stack-align.ll.bcllvm/test/Bitcode/invalid-functionptr-align.ll.bc renamed to llvm/test/Bitcode/invalid-stack-align.ll.bc llvm/test/Bitcode/invalid-functionptr-align.ll.bc renamed to llvm/test/Bitcode/invalid-stack-align.ll.bc

@@ -3,7 +3,7 @@
 ; This test formerly failed because of wrong custom lowering for
 ; fptosi of ppc_fp128.
 
-target datalayout = "E-p:32:32:32-S0-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f16:16:16-f32:32:32-f64:64:64-f128:64:128-v64:64:64-v128:128:128-a0:0:64-n32"
+target datalayout = "E-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f16:16:16-f32:32:32-f64:64:64-f128:64:128-v64:64:64-v128:128:128-a0:0:64-n32"
 target triple = "powerpc-unknown-linux-gnu"
 
 %core.time.TickDuration = type { i64 }

llvm/test/CodeGen/PowerPC/pr15359.ll

@@ -2,7 +2,7 @@
 
 ; This test formerly failed due to no handling for a ppc_fp128 undef.
 
-target datalayout = "E-p:32:32:32-S0-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f16:16:16-f32:32:32-f64:64:64-f128:64:128-v64:64:64-v128:128:128-a0:0:64-n32"
+target datalayout = "E-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f16:16:16-f32:32:32-f64:64:64-f128:64:128-v64:64:64-v128:128:128-a0:0:64-n32"
 target triple = "powerpc-unknown-linux-gnu"
 
 %core.time.TickDuration.37.125 = type { i64 }