[PATCH] D89891: [llvm-ar][Object] Fix detection of need for 64-bit archive symbol tables

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:610
+    raw_svector_ostream SymtabHeader(SymtabHeaderBuf);
+    writeSymbolTableHeader(SymtabHeader, Kind, Deterministic, SymtabSize);
+
----------------
grimar wrote:
> andrewng wrote:
> > andrewng wrote:
> > > grimar wrote:
> > > > andrewng wrote:
> > > > > grimar wrote:
> > > > > > grimar wrote:
> > > > > > > grimar wrote:
> > > > > > > > andrewng wrote:
> > > > > > > > > grimar wrote:
> > > > > > > > > > I was a bit confused when saw `writeSymbolTableHeader` here.
> > > > > > > > > > `Kind` can be changed below:
> > > > > > > > > > 
> > > > > > > > > > ```
> > > > > > > > > >     if (LastOffset >= (1ULL << Sym64Threshold)) {
> > > > > > > > > >       if (Kind == object::Archive::K_DARWIN)
> > > > > > > > > >         Kind = object::Archive::K_DARWIN64;
> > > > > > > > > >       else
> > > > > > > > > >         Kind = object::Archive::K_GNU64;
> > > > > > > > > >     }
> > > > > > > > > > ```
> > > > > > > > > > 
> > > > > > > > > > I think would be more natural to call `writeSymbolTableHeader` after last possible modification of `Kind`, i.e. after this code block. Is it possible?
> > > > > > > > > This call to `writeSymbolTableHeader` is purely used to determine the size of the header and is not a write to the output which happens later.
> > > > > > > > This wasn't addressed?
> > > > > > > > 
> > > > > > > > aside: could you please mask addressed comments as "Done"? So that reviewers can see the status.
> > > > > > > mask -> mark
> > > > > > > This call to writeSymbolTableHeader is purely used to determine the size of the header and is not a write to the output which happens later.
> > > > > > 
> > > > > > `writeSymbolTableHeader` still uses `Kind`. It is just not clean to give it one `Kind` and change it to another later.
> > > > > But this is by design. This call to `writeSymbolTableHeader` is used as part of the calculation of the 32-bit symbol table size in order to determine whether a 64-bit symbol table is required. If this is required, then the `Kind` is changed and the appropriate format is output. Does that make sense?
> > > > I believe that the behavior of original code was:
> > > > 1) Calculate the size of the symbol table.
> > > > 2) Switch to 64-bit symbol table Kind if needed.
> > > > 3) Write the header and the body of the symbol table in one move.
> > > > 
> > > > Now the behavior with this patch is:
> > > > 1) Calculate the size of the symbol table.
> > > > 2) Write the symbol table header using the `Kind` (Kind1) we have at this step.
> > > > 3) Switch to 64-bit symbol table `Kind` (Kind2) if needed.
> > > > 4) Write the body of the symbol table using the new Kind (Kind2)
> > > > 
> > > > `computeSymbolTableSize` uses `Kind` for `isBSDLike(Kind)` call.
> > > > Perhaps instead of `Kind` it could take `bool IsBSDLike`? That way we know that
> > > > changing of `Kind` from 32->64 does not affect anything. What do you think?
> > > > 
> > > Sorry if I haven't explained clearly, but the behaviour with this patch is:
> > > 
> > > 1) Correctly calculate `LastOffset` including the correct calculation for the 32-bit symbol table size and also include the file signature.
> > > 2) The above calculation includes a call to `writeSymbolTableHeader` with the original `Kind` to a temporary buffer in order to determine the symbol table header size.
> > > 3) Switch to a 64-bit symbol table `Kind` if required.
> > > 4) Write archive which includes a call to `writeSymbolTableHeader` with the final `Kind` and the actual output buffer.
> > > 
> > > Hope that clarifies the situation.
> > Perhaps it would make it clearer if I wrap this use of `writeSymbolTableHeader` (in point 2 above) in a function such as `computeSymbolTableHeaderSize`? Although this would be the only place that it would be used. What do you think?
> > Perhaps it would make it clearer if I wrap this use of writeSymbolTableHeader (in point 2 above) in a function such as computeSymbolTableHeaderSize
> 
> Makes sense I think. Probably you can just have a lambda names like that instead of having a function.
names -> named


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

https://reviews.llvm.org/D89891