[PATCH] D38302: [X86][MC] Fixed crash when assembling a file that contains section with 64-bit alignment (PR34726)

[Belochapka, Konstantin via llvm-commits]

Hi Rafael,

For some reason I cannot see your comments on the Phabricator d38302 page.

1) Do you actually have a use case for such a large alignment or you just want to fix a crash?
This bug was originated from sony internal Bugzilla and the ELF64 format supports 64 bit alignment.

2) Can you use 16ULL?
On windows64 the uint64_t is long long int , but on linux64 the uint64_t is long int,  therefore on linux  ->  std::max(long long int, long int) will cause compile error.

3) I think we are missing tests for some of these (common symbols for example).
Can you elaborate a little bit more on this issue. What tests are required?

3) Does this change the size of a MCSection? Can you assert that ALignment is a power of 2 and store the log?
Will do.



Thanks,
Konstantin

-----Original Message-----
From: Rafael Avila de Espindola [mailto:rafael.espindola at gmail.com] 
Sent: Tuesday, October 03, 2017 3:40 PM
To: reviews+D38302+public+33d9a68f0edf77b9 at reviews.llvm.org; Konstantin Belochapka via Phabricator <reviews at reviews.llvm.org>; Belochapka, Konstantin <konstantin.belochapka at sony.com>; craig.topper at gmail.com
Cc: aheejin at gmail.com; llvm-commits at lists.llvm.org
Subject: Re: [PATCH] D38302: [X86][MC] Fixed crash when assembling a file that contains section with 64-bit alignment (PR34726)

Do you actually have a use case for such a large alignment or you just want to fix a crash? If fixing a crash, it might be better to just produce a clean error saying that such a large alignment is not supported.

In case we do need to support it:

Konstantin Belochapka via Phabricator via llvm-commits <llvm-commits at lists.llvm.org> writes:

> Index: lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp
> ===================================================================
> --- lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp
> +++ lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp
> @@ -776,9 +776,9 @@
>    MCSection &BSSSection = *OFI.getBSSSection();
>    MCA.registerSection(BSSSection);
>  
> -  TextSection.setAlignment(std::max(16u, 
> TextSection.getAlignment()));
> -  DataSection.setAlignment(std::max(16u, 
> DataSection.getAlignment()));
> -  BSSSection.setAlignment(std::max(16u, BSSSection.getAlignment()));
> +  TextSection.setAlignment(std::max((uint64_t)16u, 
> + TextSection.getAlignment()));  
> + DataSection.setAlignment(std::max((uint64_t)16u, 
> + DataSection.getAlignment()));  
> + BSSSection.setAlignment(std::max((uint64_t)16u, 
> + BSSSection.getAlignment()));

Can you use 16ULL?

> Index: include/llvm/MC/MCStreamer.h
> ===================================================================
> --- include/llvm/MC/MCStreamer.h
> +++ include/llvm/MC/MCStreamer.h
> @@ -509,15 +509,15 @@
>    /// \param ByteAlignment - The alignment of the symbol if
>    /// non-zero. This must be a power of 2.
>    virtual void EmitCommonSymbol(MCSymbol *Symbol, uint64_t Size,
> -                                unsigned ByteAlignment) = 0;
> +                                uint64_t ByteAlignment) = 0;
>  
>    /// \brief Emit a local common (.lcomm) symbol.
>    ///
>    /// \param Symbol - The common symbol to emit.
>    /// \param Size - The size of the common symbol.
>    /// \param ByteAlignment - The alignment of the common symbol in bytes.
>    virtual void EmitLocalCommonSymbol(MCSymbol *Symbol, uint64_t Size,
> -                                     unsigned ByteAlignment);
> +                                     uint64_t ByteAlignment);
>  
>    /// \brief Emit the zerofill section and an optional symbol.
>    ///
> @@ -527,7 +527,7 @@
>    /// \param ByteAlignment - The alignment of the zerofill symbol if
>    /// non-zero. This must be a power of 2 on some targets.
>    virtual void EmitZerofill(MCSection *Section, MCSymbol *Symbol = nullptr,
> -                            uint64_t Size = 0, unsigned ByteAlignment = 0) = 0;
> +                            uint64_t Size = 0, uint64_t ByteAlignment 
> + = 0) = 0;
>  
>    /// \brief Emit a thread local bss (.tbss) symbol.
>    ///
> @@ -537,7 +537,7 @@
>    /// \param ByteAlignment - The alignment of the thread local common symbol
>    /// if non-zero.  This must be a power of 2 on some targets.
>    virtual void EmitTBSSSymbol(MCSection *Section, MCSymbol *Symbol,
> -                              uint64_t Size, unsigned ByteAlignment = 0);
> +                              uint64_t Size, uint64_t ByteAlignment = 
> + 0);
>  
>    /// @}
>    /// \name Generating Data
> @@ -682,9 +682,9 @@
>    /// \param MaxBytesToEmit - The maximum numbers of bytes to emit, or 0. If
>    /// the alignment cannot be reached in this many bytes, no bytes are
>    /// emitted.
> -  virtual void EmitValueToAlignment(unsigned ByteAlignment, int64_t 
> Value = 0,
> +  virtual void EmitValueToAlignment(uint64_t ByteAlignment, int64_t 
> + Value = 0,
>                                      unsigned ValueSize = 1,
> -                                    unsigned MaxBytesToEmit = 0);
> +                                    uint64_t MaxBytesToEmit = 0);
>  
>    /// \brief Emit nops until the byte alignment \p ByteAlignment is reached.
>    ///
> @@ -696,8 +696,8 @@
>    /// \param MaxBytesToEmit - The maximum numbers of bytes to emit, or 0. If
>    /// the alignment cannot be reached in this many bytes, no bytes are
>    /// emitted.
> -  virtual void EmitCodeAlignment(unsigned ByteAlignment,
> -                                 unsigned MaxBytesToEmit = 0);
> +  virtual void EmitCodeAlignment(uint64_t ByteAlignment,
> +                                 uint64_t MaxBytesToEmit = 0);

I think we are missing tests for some of these (common symbols for example).

>    /// \brief Emit some number of copies of \p Value until the byte offset \p
>    /// Offset is reached.
> Index: include/llvm/MC/MCSection.h
> ===================================================================
> --- include/llvm/MC/MCSection.h
> +++ include/llvm/MC/MCSection.h
> @@ -59,7 +59,7 @@
>    MCSymbol *Begin;
>    MCSymbol *End = nullptr;
>    /// The alignment requirement of this section.
> -  unsigned Alignment = 1;
> +  uint64_t Alignment = 1;
>    /// The section index in the assemblers section list.
>    unsigned Ordinal = 0;
>    /// The index of this section in the layout order.
> @@ -114,8 +114,8 @@
>    MCSymbol *getEndSymbol(MCContext &Ctx);
>    bool hasEnded() const;
>  
> -  unsigned getAlignment() const { return Alignment; }
> -  void setAlignment(unsigned Value) { Alignment = Value; }
> +  uint64_t getAlignment() const { return Alignment; }  void 
> + setAlignment(uint64_t Value) { Alignment = Value; }

Does this change the size of a MCSection? Can you assert that ALignment is a power of 2 and store the log?

> Index: include/llvm/MC/MCFragment.h
> ===================================================================
> --- include/llvm/MC/MCFragment.h
> +++ include/llvm/MC/MCFragment.h
> @@ -278,7 +278,7 @@
>  
>  class MCAlignFragment : public MCFragment {
>    /// Alignment - The alignment to ensure, in bytes.
> -  unsigned Alignment;
> +  uint64_t Alignment;

Same as above.

Cheers,
Rafael