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

[Rafael Avila de Espindola via llvm-commits]

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