[PATCH] D152945: [SystemZ][z/OS] Implement executePostLayoutBinding for GOFFWriter

[Digger Lin via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:705
+    Flags Attr[10];
+    void setAmode(GOFF::ESDAmode Amode) { Attr[0].set(0, 8, Amode); }
+    void setRmode(GOFF::ESDRmode Rmode) { Attr[1].set(0, 8, Rmode); }
----------------
Everybody0523 wrote:
> DiggerLin wrote:
> > I am expressing the difficulty in understanding the meaning or significance of the values 0, 8 in a particular context. it's possible to use a "static constexpr" to define these values to provide more clarity and context? 
> > 
> > if can , please change all the following .
> > 
> > for example: in xcoff.h
> > 
> > constexpr size_t FileNamePadSize = 6;
> > constexpr size_t NameSize = 8;
> > constexpr size_t FileHeaderSize32 = 20;
> > constexpr size_t FileHeaderSize64 = 24;
> > 
> So in the highlighted function (`setAmode`), we're setting the address mode of the symbol, which is an 8-bit field. The first parameter of the `set()` function is the offset to which we begin, the second parameter is the number of bits that should be written, and the last value is the actual value to write, so in this case we just mean "0-bit offset" and "write 8 bits" for the 0 and 8 respectively.

sorry that I do not express my comment clearly. My worry about is that your code is not easy to understand  without reading the GOFF document.

can we change the code something like ?

```
struct SymbolBehavAttr {
  uint8_t AddressingProperties;
  uint8_t ResidenceProperties;
  uint8_t TextRecordtyle:4;
  uint8_t BindingAlgorithm:4;
  ....
}  ;
```
 (and if the SymbolBehavAttr  will be by other functionality later, we  can put it GOFF.h ?)

and then you do not need the API setRmode() and setAmode()
and you can assign the value directly

SymbolBehavAttr  BehavAttr; 
BehavAttr.AddressingProperties = ...



================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:739
+  } BehavAttr;
+
+  uint32_t AdaEsdId = 0;
----------------
DiggerLin wrote:
> do not use inline struct here, using lambda functions here.
please ignore my about  comment.


================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:874
+  OS.writeBE<uint64_t>(0);                      // Reserved
+  for (auto F : BehavAttr.Attr)
+    OS.writeBE<uint8_t>(F);          // Behavioral Attributes
----------------
can you add a new api for GOFFOstream for  the size which large than 64bits ?
something like 

https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Support/EndianStream.h#L46  ?


================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:911
+          Data.append(1, '\0');
+        }
+        bufferText(Section, SectionOffset, Data);
----------------
change  
    for (unsigned I = 0; I < Size; ++I) {
          Data.append(1, '\0');
to  
    Data.append(Size, '\0');


================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:937
+          Data.append(1, B);
+          I++;
+          if (I == MaxChunkSize) {
----------------
change to ++I;


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152945/new/

https://reviews.llvm.org/D152945