[PATCH] MC: Implement COFF .linkonce directive

Nico Rieck nico.rieck at gmail.com
Fri Jul 5 06:27:56 PDT 2013



================
Comment at: docs/Extensions.rst:42-43
@@ -39,1 +41,4 @@
     .long $unwind$fun at imgrel
+
+
+``.linkonce`` Directive
----------------
Rafael Ávila de Espíndola wrote:
> This is the same as the gnu assembler, no? Do we consider it an extension because it is not what the native tools do?
GAS doesn't support associative, largest and newest. Also, the GAS documentation is wrong when it mentions warnings for the others, at least according to the PECOFF spec and Microsoft's linker, which issues errors.

================
Comment at: include/llvm/MC/MCSectionCOFF.h:39
@@ -35,1 +38,3 @@
+    /// associative Selection (IMAGE_COMDAT_SELECT_ASSOCIATIVE).
+    mutable const MCSectionCOFF *Assoc;
 
----------------
Rafael Ávila de Espíndola wrote:
> Why these fields need to be mutable? The equivalent ones in MCSectionELF are not.
I agree mutable is not ideal, but MCStreamer::getCurrentSection() returns only pointers to const and I don't see any other way to retrieve the current section. This is also the only instance of a setter on a section type. How would you prefer this to be handled? Should I change MCStreamer's interface and add a suitable method? Or just const_cast the returned pointer?

================
Comment at: lib/MC/MCParser/COFFAsmParser.cpp:458
@@ +457,3 @@
+
+  Current->setSelection(Type, Assoc);
+
----------------
Rafael Ávila de Espíndola wrote:
> What is the correct behavior if  .linkonce is specified twice, should we produce an error?
We probably should. GAS doesn't specify this, and allows it without any warnings or errors. But the behavior depends on the order of internal constants.

================
Comment at: lib/MC/MCSectionCOFF.cpp:38
@@ +37,3 @@
+  this->Assoc = Assoc;
+  if (Selection != 0)
+    Characteristics |= COFF::IMAGE_SCN_LNK_COMDAT;
----------------
Rafael Ávila de Espíndola wrote:
> We never pass 0, it would be better to assert that.
Agreed.


http://llvm-reviews.chandlerc.com/D1100



More information about the llvm-commits mailing list