This is an archive of the discontinued LLVM Phabricator instance.

[X86] Fix variadic argument handling for x32
ClosedPublic

Authored by hvdijk on Dec 12 2020, 4:39 AM.

Details

Summary

The X86-64 ABI defines va_list as

typedef struct {
  unsigned int gp_offset;
  unsigned int fp_offset;
  void *overflow_arg_area;
  void *reg_save_area;
} va_list[1];

This means the size, alignment, and reg_save_area offset will depend on
whether we are in LP64 or in ILP32 mode, so this commit adds the checks.
Additionally, the VAARG_64 pseudo-instruction assumed 64-bit pointers, so
this commit adds a VAARG_X32 pseudo-instruction that behaves just like
VAARG_64, except for assuming 32-bit pointers.

Some of these changes were originally done by
Michael Liao <michael.hliao@gmail.com>.

Fixes https://bugs.llvm.org/show_bug.cgi?id=48428.

Diff Detail

Event Timeline

hvdijk created this revision.Dec 12 2020, 4:39 AM
hvdijk requested review of this revision.Dec 12 2020, 4:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 12 2020, 4:39 AM

Its OK to commit the asm.py / update_llc_test_checks.py changes right away to split them from the rest of this patch

llvm/lib/Target/X86/X86ISelLowering.cpp
31558

update comment

Its OK to commit the asm.py / update_llc_test_checks.py changes right away to split them from the rest of this patch

Thanks, done.

llvm/lib/Target/X86/X86ISelLowering.cpp
31558

This comment looks correct to me in its current form. I'm not sure if you mean to change va_arg or X86-64, but va_arg is the spelling that the LLVM IR instruction uses, and X86-64 includes both the LP64 and ILP32 ABIs. If I am missing something here, please let me know what.

yubing added a subscriber: yubing.Dec 12 2020, 9:34 PM
RKSimon added inline comments.Dec 13 2020, 8:40 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
31558

OK - I was thinking that maybe we should explicitly mention that we're trying to handle x32 here as well - but I agree it was rather pedantic!

hvdijk updated this revision to Diff 311450.Dec 13 2020, 9:07 AM
hvdijk edited the summary of this revision. (Show Details)

rebase?

Sure, rebased to take out the already applied update to UpdateTestChecks.

RKSimon added inline comments.Dec 13 2020, 9:57 AM
llvm/test/CodeGen/X86/x86-64-varargs.ll
1–3

Please can you regenerate + commit this test file with the no_x86_scrub_sp arg just to confirm theres no diff in the X64 codegen - then rebase adding the X32 run

hvdijk updated this revision to Diff 311460.Dec 13 2020, 10:39 AM

Rebase again, taking the X64 changes out of the diff.

hvdijk marked an inline comment as done.Dec 13 2020, 10:39 AM
hvdijk added inline comments.
llvm/test/CodeGen/X86/x86-64-varargs.ll
1–3

Sure, done.

RKSimon accepted this revision.Dec 14 2020, 4:08 AM

LGTM - cheers

This revision is now accepted and ready to land.Dec 14 2020, 4:08 AM
This revision was landed with ongoing or failed builds.Dec 14 2020, 3:47 PM
This revision was automatically updated to reflect the committed changes.
hvdijk marked an inline comment as done.