[llvm-readobj] Only allow 4-byte pr_data

[MaskRay]

Summary

AMD64 psABI says: "The pr_data field of each property contains a 4-byte unsigned integer." Thus we don't need to handle 8-byte pr_data.

[xiangzhangllvm]

Yes, there is a similar patch for the data size in https://reviews.llvm.org/D56080, which deal with the size in X86 llvm asmprint.

[MaskRay]

In D58103#1394340, @xiangzhangllvm wrote:

Yes, there is a similar patch for the data size in https://reviews.llvm.org/D56080, which deal with the size in X86 llvm asmprint.

So it is always 4, correct? I just checked binutils-gdb and it also uses fixed 4.

[grimar]

Looks correct, FTR ABI link is https://github.com/hjl-tools/x86-psABI/wiki/x86-64-psABI-draft.pdf.

Regarding the refactoring, in my opinion, the original code was simpler. I would leave it as is.
(at least refactoring change could be in a separate patch, if you want to do it)

[MaskRay]

In D58103#1394386, @grimar wrote:

Looks correct, FTR ABI link is https://github.com/hjl-tools/x86-psABI/wiki/x86-64-psABI-draft.pdf.

Regarding the refactoring, in my opinion, the original code was simpler. I would leave it as is.
(at least refactoring change could be in a separate patch, if you want to do it)

For GNU_PROPERTY_X86_FEATURE_1_AND, there are only 2 bits. When people start implementing
GNU_PROPERTY_X86_ISA_1_USED GNU_PROPERTY_X86_ISA_2_USED GNU_PROPERTY_X86_COMPAT_ISA_1_USED, this style will become better :)

[xiangzhangllvm]

Yes it is always 4 now

[grimar]

In D58103#1394387, @MaskRay wrote:

In D58103#1394386, @grimar wrote:

Looks correct, FTR ABI link is https://github.com/hjl-tools/x86-psABI/wiki/x86-64-psABI-draft.pdf.

Regarding the refactoring, in my opinion, the original code was simpler. I would leave it as is.
(at least refactoring change could be in a separate patch, if you want to do it)

For GNU_PROPERTY_X86_FEATURE_1_AND, there are only 2 bits. When people start implementing
GNU_PROPERTY_X86_ISA_1_USED GNU_PROPERTY_X86_ISA_2_USED GNU_PROPERTY_X86_COMPAT_ISA_1_USED, this style will become better :)

There are different ways to do a refactoring here. For example, it could probably be:

auto DumpFlag = [](StringRef Name, uint64_t Flag, uint32_t &Val) {
  if (Val & Flag) {
    Val &= ~Flag;
    OS << Name;
    if (Val)
      OS << ", ";
  }
};

DumpFlag("IBT", GNU_PROPERTY_X86_FEATURE_1_IBT, pr_data);
DumpFlag("SHSTK", GNU_PROPERTY_X86_FEATURE_1_SHSTK, pr_data);

Which still looks simpler to me, but you or other people might disagree,
so probably it is better to discuss the refactoring in a different review.

My main point is that functional and refactoring changes
should not be mixed in the same patch.

[MaskRay]

Remove refactoring

[MaskRay]

.

[grimar]

LGTM

[MaskRay]

Your approach looks great! Moved the refactoring part (I also have to implement GNU_PROPERTY_X86_FEATURE_2_{NEEDED,USED} to demonstrate the idea) to D58112

llvm/trunk/test/tools/llvm-readobj/note-gnu-property.s

 // GNU-NEXT:     X86 features: SHSTK
 // GNU-NEXT:     X86 features: IBT, SHSTK
 // GNU-NEXT:     X86 features: none
 // GNU-NEXT:     <application-specific type 0xfefefefe>
 // GNU-NEXT:     stack size: <corrupt length: 0x0>
 // GNU-NEXT:     stack size: <corrupt length: 0x4>
 // GNU-NEXT:     no copy on protected <corrupt length: 0x1>
 // GNU-NEXT:     X86 features: <corrupt length: 0x0>
-// GNU-NEXT:     X86 features: IBT, <unknown flags: 0xf000f000f000f000>
+// GNU-NEXT:     X86 features: IBT, <unknown flags: 0xf000f000>
 // GNU-NEXT:     <corrupt type (0x2) datasz: 0x1>
 
 // LLVM:      Notes [
 // LLVM-NEXT:   NoteSection {
 // LLVM-NEXT:     Offset: 0x40
 // LLVM-NEXT:     Size: 0xB8
 // LLVM-NEXT:     Note {
 // LLVM-NEXT:       Owner: GNU
 // LLVM-NEXT:       Data size: 0xA8
 // LLVM-NEXT:       Type: NT_GNU_PROPERTY_TYPE_0 (property note)
 // LLVM-NEXT:       Property [
 // LLVM-NEXT:         stack size: 0x100
 // LLVM-NEXT:         stack size: 0x100
 // LLVM-NEXT:         no copy on protected
 // LLVM-NEXT:         X86 features: SHSTK
 // LLVM-NEXT:         X86 features: IBT, SHSTK
 // LLVM-NEXT:         X86 features: none
 // LLVM-NEXT:         <application-specific type 0xfefefefe>
 // LLVM-NEXT:         stack size: <corrupt length: 0x0>
 // LLVM-NEXT:         stack size: <corrupt length: 0x4>
 // LLVM-NEXT:         no copy on protected <corrupt length: 0x1>
 // LLVM-NEXT:         X86 features: <corrupt length: 0x0>
-// LLVM-NEXT:         X86 features: IBT, <unknown flags: 0xf000f000f000f000>
+// LLVM-NEXT:         X86 features: IBT, <unknown flags: 0xf000f000>
 // LLVM-NEXT:         <corrupt type (0x2) datasz: 0x1>
 // LLVM-NEXT:       ]
 // LLVM-NEXT:     }
 // LLVM-NEXT:   }
 // LLVM-NEXT: ]
 
 .section ".note.gnu.property", "a"
 .align 4 
begin:
   .p2align 3        /* Align to 8 byte for 64 bit */
   
   .long 2           /* Type: GNU_PROPERTY_NO_COPY_ON_PROTECTED */
   .long 0           /* Data size */
   .p2align 3        /* Align to 8 byte for 64 bit */
 
   /* CET property note */
   .long 0xc0000002  /* Type: GNU_PROPERTY_X86_FEATURE_1_AND */
-  .long 8           /* Data size */
-  .quad 2           /* GNU_PROPERTY_X86_FEATURE_1_SHSTK */
+  .long 4           /* Data size */
+  .long 2           /* GNU_PROPERTY_X86_FEATURE_1_SHSTK */
   .p2align 3        /* Align to 8 byte for 64 bit */
 
   /* CET property note with padding */
   .long 0xc0000002  /* Type: GNU_PROPERTY_X86_FEATURE_1_AND */
   .long 4           /* Data size */
   .long 3           /* Full CET support */
   .p2align 3        /* Align to 8 byte for 64 bit */
 
   .long 0xc0000002  /* Type: GNU_PROPERTY_X86_FEATURE_1_AND */
-  .long 8           /* Data size */
-  .quad 0           /* Empty flags, not an error */
+  .long 4           /* Data size */
+  .long 0           /* Empty flags, not an error */
   .p2align 3        /* Align to 8 byte for 64 bit */
   
   /* All notes below are broken. Test we are able to report them. */
   
   /* Broken note type */
   .long 0xfefefefe  /* Invalid type for testing */
   .long 0           /* Data size */
   .p2align 3        /* Align to 8 byte for 64 bit */
begin:
 
   /* CET note with size zero */
   .long 0xc0000002  /* Type: GNU_PROPERTY_X86_FEATURE_1_AND */
   .long 0           /* Data size */
   .p2align 3        /* Align to 8 byte for 64 bit */
 
   /* CET note with bad flags */
   .long 0xc0000002         /* Type: GNU_PROPERTY_X86_FEATURE_1_AND */
-  .long 8                  /* Data size */
-  .quad 0xf000f000f000f001 /* GNU_PROPERTY_X86_FEATURE_1_IBT and bad bits */
+  .long 4                  /* Data size */
+  .long 0xf000f001         /* GNU_PROPERTY_X86_FEATURE_1_IBT and bad bits */
   .p2align 3               /* Align to 8 byte for 64 bit */
   
   /* GNU_PROPERTY_NO_COPY_ON_PROTECTED with pr_datasz and without data */
   .long 2           /* Type: GNU_PROPERTY_NO_COPY_ON_PROTECTED */
   .long 1           /* Data size (corrupted) */
   .p2align 3        /* Align to 8 byte for 64 bit */
 end:

llvm/trunk/tools/llvm-readobj/ELFDumper.cpp

static std::string getGNUProperty(uint32_t Type, uint32_t DataSize,
   }
   case GNU_PROPERTY_NO_COPY_ON_PROTECTED:
     OS << "no copy on protected";
     if (DataSize)
       OS << format(" <corrupt length: 0x%x>", DataSize);
     return OS.str();
   case GNU_PROPERTY_X86_FEATURE_1_AND:
     OS << "X86 features: ";
-    if (DataSize != 4 && DataSize != 8) {
+    if (DataSize != 4) {
       OS << format("<corrupt length: 0x%x>", DataSize);
       return OS.str();
     }
-    uint64_t CFProtection =
-        (DataSize == 4)
-            ? support::endian::read32<ELFT::TargetEndianness>(Data.data())
-            : support::endian::read64<ELFT::TargetEndianness>(Data.data());
+    uint32_t CFProtection  =
+        support::endian::read32<ELFT::TargetEndianness>(Data.data());
     if (CFProtection == 0) {
       OS << "none";
       return OS.str();
     }
     if (CFProtection & GNU_PROPERTY_X86_FEATURE_1_IBT) {
       OS << "IBT";
       CFProtection &= ~GNU_PROPERTY_X86_FEATURE_1_IBT;
       if (CFProtection)
         OS << ", ";
     }
     if (CFProtection & GNU_PROPERTY_X86_FEATURE_1_SHSTK) {
       OS << "SHSTK";
       CFProtection &= ~GNU_PROPERTY_X86_FEATURE_1_SHSTK;
       if (CFProtection)
         OS << ", ";
     }
     if (CFProtection)
-      OS << format("<unknown flags: 0x%llx>", CFProtection);
+      OS << format("<unknown flags: 0x%x>", CFProtection);
     return OS.str();
   }
 }
 
 template <typename ELFT>
 static SmallVector<std::string, 4>
 getGNUPropertyList(ArrayRef<uint8_t> Arr) {
   using Elf_Word = typename ELFT::Word;