[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:
> Everybody0523 wrote:
> > DiggerLin wrote:
> > > 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 = ...
> > > 
> > Hi, so I guess you would want something like this? 
> > 
> > ```
> >   LLVM_PACKED_START
> >   struct SymbolBehavAttr {
> >     uint8_t AddressingProperties;
> >     uint8_t ResidenceProperties;
> >     uint8_t TextRecordStyle:4; //Byte 2 begin
> >     uint8_t BindingAlgorithm:4;
> >     uint8_t TaskingBehavior:3; //Byte 3 begin
> >     uint8_t ReservedSpace0:1;
> >     uint8_t ReadOnlyIndicator:1;
> >     uint8_t Executable:3;
> >     uint8_t ReservedSpace1:2; //Byte 4 begin
> >     uint8_t DuplicateSymbolSeverity:2;
> >     uint8_t BindingStrength:4; 
> >     uint8_t ClassLoadingBehavior:2; //Byte 5 begin
> >     uint8_t CommonFlag:1;
> >     uint8_t IsDirectReference:1;
> >     uint8_t BindingScope:4;
> >     uint8_t ReservedSpace2:3; //Byte 6 begin
> >     uint8_t LinkageType:1;
> >     uint8_t Alignment:5;
> >     uint8_t ReservedSpace3; 
> >     uint16_t ReservedSpace4;
> >   };
> >   LLVM_PACKED_END
> > ```
> (meant to send htis as part of above message, idk what happened ¯\_(ツ)_/¯)
> 
> I can see how this self-documents the format of the Behavior Attributes field in GOFF, which is nice, but I do find it annoying that I'd need to fill out the reserved bytes in order to ensure the format is correct...
> 
> 
yes. in xcoff, there is example:


```
struct XCOFFBlockAuxEnt32 {
  uint8_t ReservedZeros1[2];
  support::ubig16_t LineNumHi;
  support::ubig16_t LineNumLo;
  uint8_t ReservedZeros2[12];
};
```

we do not care about  the value of the reserved bytes since it is resevered. is it special in GOFF ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152945