[PATCH] D35152: Add some basic linker module symbols

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/COFF/Config.h:95
   llvm::SmallString<128> PDBPath;
+  std::vector<StringRef> Argv;
 
----------------
zturner wrote:
> ruiu wrote:
> > This can be ArrayRef.
> In order to make it `ArrayRef` I have to make it `ArrayRef<const char *>`.  This is what I had in an earlier version of the patch, but I had problems on the call to `llvm::join()` (because you can't use `operator+` on two `const char*`.  So if I want to be able to use `StringRef`, it has to be `std::vector`
Makes sense.


================
Comment at: lld/COFF/PDB.cpp:411
+
+  CS.Machine = CPUType::Intel80386;
+  CS.Flags = CompileSym3Flags::None;
----------------
zturner wrote:
> zturner wrote:
> > smeenai wrote:
> > > zturner wrote:
> > > > ruiu wrote:
> > > > > Is this value the same on x86-64?
> > > > That's a good question, I should check.
> > > Probably dumb question: what about ARM (and AArch64 in the future)?
> > Whoever's interested in adding PDB support for `ARM and `AArch64` can update this code :)  The `CPUType` enumeration has a bunch of ARM values in the enumeration:
> > 
> > ```
> >   ARM3 = 0x60,
> >   ARM4 = 0x61,
> >   ARM4T = 0x62,
> >   ARM5 = 0x63,
> >   ARM5T = 0x64,
> >   ARM6 = 0x65,
> >   ARM_XMAC = 0x66,
> >   ARM_WMMX = 0x67,
> >   ARM7 = 0x68,
> >   ARMNT = 0xf4,
> > ```
> > 
> > the differences between which are not obvious to me :)  But the support is there (in theory) for whoever wants this.
> Actually `ARM64` is probably `ARM8`, which does not seem to be represented in this enumeration.  So there will be some head scratching involved to figure out what to do there
Agreed. We can change this later when someone wants PDB support for ARM/ARM64.


https://reviews.llvm.org/D35152