[llvm-commits] [llvm] r168704 - in /llvm/trunk: lib/CodeGen/AsmPrinter/AsmPrinter.cpp test/CodeGen/ARM/2010-12-15-elf-lcomm.ll test/CodeGen/ARM/elf-lcomm-align.ll

[Ulrich Weigand]

Rafael Espíndola <rafael.espindola at gmail.com> wrote on 29.11.2012 17:23:45:

> > As to 1., if we want to achieve full compatibility with gas, we'd have
> > to implement the precise rules (platform-specific) default alignment
> > rules gas uses.  This would be done in AsmParser::ParseDirectiveComm,
> > I assume.  [ But this doesn't matter either way for the case I was
> > fixing, which is generating code for a compiler-generated global
> > variable via AsmPrinter::EmitGlobalVariable. ]
>
> IMHO this is desirable. Switching to or from the integrated assembly
> to the system external one should not change the meaning for the
> program. In particular, it would probably make issues like this easier
> to fix.

Just to clarify: this point affects only parsing assembler source files.
It does not change anything whatsoever in the behaviour of *compiling*
C source files and switching between the integated and external assembler
there.  This is why I don't see how this would affect my problem either
way ...

> > As to 2., since EmitLocalCommonSymbol always gets a ByteAlignment
> > argument, it is arguably correct to precisely honor that argument
> > in the MCELFStreamer case.  The MCAsmStreamer case is the difficult
> > one on platforms where the native assembler doesn't support an
> > alignment argument with its .lcomm directive.  In this case, it is
> > impossible to implement EmitLocalCommonSymbol precisely, except
> > for very specific values of ByteAlignment.  This means that in
> > almost all cases, it would still be incorrect for EmitGlobalVariable
> > to call EmitLocalCommonSymbol, and since there is no particular
> > advantage to doing so in the rare cases where it would be correct,
> > the patch I checked in still seems the best solution ...
>
> So it is the native assembler that was producing a broken binary?

Yes.  (Well, "broken" in the sense that it contains unnecessarily
strong alignment requirements for variables.  It will still *work*.)

> If
> so your patch is correct, I had understood the other way: That codegen
> was assuming the behavior of the external one and so the integrated
> one was producing broken binaries.

No, the integrated assembler was doing it correctly, i.e. not imposing
unnecessary alignment.  (In other words, using clang + integrated
assembler I was getting the same binary as with GCC; using clang +
external assembler I was getting a different binary.)

Bye,
Ulrich